Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve flexibility of Slack connector #4812
Improve flexibility of Slack connector #4812
Changes from 13 commits
6eca764
f5aa081
aec05aa
3450447
6a5e21b
559b727
c6cc6ac
995cace
097dc97
d5907f9
aecdd77
f11754f
8c710e5
3df6fac
b622f8d
d98c742
1f6d347
3ffd78d
0a6fa56
a1ae358
bedab05
c4f0f8f
ea859a3
8a0cc26
cf27402
167c055
8f96d03
9bf680b
78702ea
e46f14b
f2d501f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid repeating the
type
in the docstring. Makes it harder to maintain if we ever change the return types. Note that you probably have to reformat my suggestion cause it's too long for a single line.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kearnsw Have you seen that one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it feels a bit weird that the
metadata
also returnssender
andtext
. The metadata is meant to provide additional information around a message.text
andsender
are the message. So in my opinion it would be a bit cleaner to keep this separated, what you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do we need this condition for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a holdover from the original codebase. I added the checks for direct message and app mention events. I think the response text should probably be set to "Received message on unsupported channel" or something to that effect instead of a blank response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok 👍 But then let's move it up to the condition in line
386
right? you could also write a helper functionis_supported_channel(output, metadata)
and then have a condition like that:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather log the "unsupport channel" case instead of returning it as part response to the HTTP request, because Slack is just throwing away this message, while
logger.warning(...)
will show it to the user.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: I'd turn it around (
if is None else ...
) because it's a bit easier to read (you don't have to negate in your head), but that's really just a personal taseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should still delete the lines after
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the test 🚀