Switch to slack_sdk and add support to ignore users and send files #428
Conversation
|
Addresses #426 |
|
Forgot I was supposed to tag @Ashex with this. |
|
I suppose I should point out: this code is what we've been running in production for some time now (most of it for more than 3 months). The ability to ignore users is newer, though: only 3 weeks or so. We had a couple of bots get into a loop, and this was an easy fix. |
|
I've pushed an update that should address all of the raised concerns. |
|
Another note for reference, the introduction of f-strings will force us to only support python 3.6+ therefore satisfying #417 |
Ashex
changed the title
There was a problem hiding this comment.
Reviewed the slack backend specifically (that I missed on my first run through as github didn't render the diff). This is a monumental rewrite and it looks great. I don't want to rush getting this merged in so I'll leave the rest of the contributors to review and provide feedback through the weekend.
I'll spin up a test instance with this branch on monday and run will through some basic tests to validate.
|
The send_file code is all gone at this point.
…On Fri, Mar 12, 2021 at 4:41 PM Ron Hudson ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In will/backends/io_adapters/slack.py
<#428 (comment)>:
>
- kwargs = {}
- if "kwargs" in event:
- kwargs.update(**event.kwargs)
-
- if hasattr(event, "source_message") and event.source_message and "channel" not in kwargs:
- self.send_message(event)
- else:
- # Came from webhook/etc
- # TODO: finish this.
- target_channel = kwargs.get("room", kwargs.get("channel", None))
- if target_channel:
- event.channel = self.get_channel_from_name(target_channel)
- if event.channel:
+ def send_file(self, event):
I get what you’re saying, but I think if we want file support and this
works then merge it in. At this point this project is almost dead because
of stagnation and lack of merging pull requests. For the health of the
project I think we’d be better to merge some stuff even if it’s not quite
perfect just to keep the ball rolling. We can always fix bugs if we
discover them as long as things are getting regularly merged.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#428 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAC5MS2Y5KNS7TI3OJ5PXXLTDKKDLANCNFSM4ZA2EKQA>
.
|
|
My two cents: There's precedent to backends having exclusive features, if they're not something that can be abstracted generally, or they're not common, etc. Merging with files and then building up an abstraction from there would seem sane to me. That said, keeping things moving does seem to be the highest priority. @BrianGallew Thanks for sending this. As you know, it's a community effort: folks here (including me) are fitting this in between their jobs, their families, relationships, doctor's appointments, etc. It takes time to build consensus, and asynchronous, community-based PRs take extra time. Your patience is appreciated, and it makes things much easier. Also, it's hard to tell tone on the internet. Please give a little extra kindness and patience to the process, like I'm sure you'd appreciate if you were on the other side! @Ashex @pastorhudson @rsalmond - do we need any changes to contributor/owner status for the repo or pypi to get this out? I've got for sure solid internet for the next couple of days, and really happy to get it done. |
|
I'm happy to do whatever you guys need |
|
I'm not angry. Like the rest of you, I'm busy with my own life. I'm quite happy to contribute to will. I'm simply uninterested in breaking things out into multiple commits/PRs. If the send_file code I submitted was unacceptable for any reason, then there's no reason for me to re-submit it. As Ashex has pointed out, there's an existing PR covering that, and potentially other implementations out there to choose from. |
|
@BrianGallew Based off everyones feedback can you add the send_file implementation back? I truly appreciate the work you've done and am grateful for this pull request, my aim was to not disrupt the work others have done but at the end of the day those are custom changes and the PR I referenced has been sitting for some time (and doesn't support DM). I already looked at the code for it when it was present so I'll approve it after I've tested everything and we'll get it merged. @skoczen I'll need pypi access so I can cut a 2.2.0 release with this PR and everything missing since 2.1.3 was released. |
|
send_file is back in and I think I kept all of the other requested changes. |
|
Can you also add it back in |
|
Sorry, my weak git-fu is now revealed. I had actually reverted all my changes, including the ones I had wanted to retain. They're in there now, and according to grep send_file is now declared in both slack.py and plugin.py. |
No worries. It happens to us all! |
|
No worries at all! I'll commence testing tomorrow :) I'll write a plugin for executing a variety of actions and share the gist with the approval. |
|
@Ashex Yep, you got it on Pypi - what's your pypi username? |
|
@Ashex you've been added to Pypi, and should be able to release as soon as you accept the invite! |
There was a problem hiding this comment.
I ran this through some tests, the only thing that needs to be fixed is adding this back to will/plugin.py as otherwise send_file fails.
FILENAME_CLEANER = re.compile(r'[^-_0-9a-zA-Z]+')
And putting aiohttp back into requirements as it turns out it is needed since we're using the async api. The line should be aiohttp>=3.7.3,<4 in order to pin the version.
Here's the plugin I wrote to do some quick tests, I wanted to ask if send_file supports DM as I was not able to get that to function.
https://gist.github.com/Ashex/cb66194c429fb8d4d2f19a15fcc75b64
There's a couple minor things I spotted but I'll correct that myself through another PR.
|
FILENAME_CLEANER re-added. And, no, I don't believe it supports DMs correctly: there is a nuance which I've not investigated sufficiently to make it work correctly in that case. |
|
Great, can you also add aiohttp back to the requirements file? I'll get this merged in once that's done. |
|
Regarding DM support, it does work when interacting with the bot directly. There's just some extra magic required when you want to send a user an unsolicited DM with a file, I did sort it out elsewhere so expect a PR in the near future with the enhancement. |
This reverts commit 299d348.
|
aiohttp change reverted |
There was a problem hiding this comment.
Thank you again for your patience and all the work you put in to rewrite this, I can definitely say you've brought life back to the project.
I'll now approve and get this merged. We have some work left to do before we cut a new release with this but it will be coming soon.
Ashex
changed the title
|
I totally agree with @Ashex thank you for the work you put into this @BrianGallew! I really appreciate it. I’m excited to see this project moving again! 🥳 |
|
:victory: |
) * Switch to slack_sdk, various extensions * Use Slack Events API * Added file upload support * Ignore users via config setting * Bump version for upcoming release * pep8 compliance * Drop support for python 2.7 * Whole lot of housekeeping Co-authored-by: Brian Gallew <[email protected]>
This is huge, I know. Sorry.
Summary: