Add typing/modernizations to BaseChatIntegration and allow using emoji unicode blocks in chats.
Review Request #13749 — Created April 22, 2024 and updated
This change mainly adds typing and modernizations to our chat integration
class. Additionally, we deprecate non-keyword arguments in most of their
methods for easier expansion in the future.We also give subclasses the option to use unicode blocks to represent
emojis in chat messages instead of shortcodes. This will be useful for the
upcoming Microsoft Teams integration, because Microsoft Teams can't interpret
emoji short codes.
Ran unit tests.
Summary | ID |
---|---|
43fe547f6061a1be30f0129470e901197fae7026 |
Description | From | Last Updated |
---|---|---|
These are class properties, not constants, so they should be defined with lower case names. We should also type them … |
david | |
The base class should probably use integration-assets/chat-common/ |
david | |
We've generally avoided python's ternary form. Can we just do if/else for these? |
david | |
We can use type instead of Type. This should probably also be ClassVar[type[...]] |
david | |
Now that we changed the base class, this probably needs to override assets_base_url to point to the slack subdir. |
david | |
This is no longer correct. |
chipx86 | |
Can we make this a set? That'll be cheaper to look up file extensions in. |
chipx86 | |
Can we start moving this over to keyword-only arguments, and use housekeeping to deprecate? I think we're using the right … |
chipx86 | |
Same here. |
chipx86 | |
Same here, though we do have callers using review_request positionally. We can just make that positional and let everything else … |
chipx86 | |
line too long (81 > 79 characters) Column: 80 Error code: E501 |
reviewbot | |
line too long (84 > 79 characters) Column: 80 Error code: E501 |
reviewbot | |
This is an existing issue, but I noticed we have a space at the end and beginning of these strings, … |
chipx86 | |
Can you pull this out into a local variable? We access it a lot, here and below. |
chipx86 | |
Let's use Sequence instead of list, unless we need to mutate it. This solves some typing issues that can come … |
chipx86 | |
I'm curious, what happens if you try: return JSONDict( attachments=[attachment], icon_url=integrations.logo_url ) Does it still complain? Can I see the … |
chipx86 |
-
-
rbintegrations/basechat/integration.py (Diff revision 1) These are class properties, not constants, so they should be defined with lower case names. We should also type them as such:
assets_base_url: ClassVar[str] = '...'
-
rbintegrations/basechat/integration.py (Diff revision 1) The base class should probably use integration-assets/chat-common/
-
rbintegrations/basechat/integration.py (Diff revision 1) We've generally avoided python's ternary form. Can we just do if/else for these?
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+828 -606) |
Checks run (2 succeeded)
-
-
rbintegrations/basechat/integration.py (Diff revision 2) We can use
type
instead ofType
. This should probably also beClassVar[type[...]]
-
rbintegrations/slack/integration.py (Diff revision 2) Now that we changed the base class, this probably needs to override assets_base_url to point to the slack subdir.
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+834 -604) |
Checks run (2 succeeded)
-
-
-
rbintegrations/basechat/integration.py (Diff revision 3) Can we make this a
set
? That'll be cheaper to look up file extensions in. -
rbintegrations/basechat/integration.py (Diff revision 3) Can we start moving this over to keyword-only arguments, and use housekeeping to deprecate?
I think we're using the right things internally, so it should be a small change.
-
-
rbintegrations/basechat/integration.py (Diff revision 3) Same here, though we do have callers using
review_request
positionally. We can just make that positional and let everything else be a keyword-only argument.Maybe the rest of these, too. Work toward a better foundation for future expansion.
By the way, feel free to schedule these as separate changes. I don't want to hold this up unnecessarily.
Change Summary:
- Make most arguments keyword only and add deprecation warnings.
- Add typing to the rest of the chat integration classes.
- Add a unit test for when slack attempts to send a notification when no webhookURL is configured.
Description: |
|
||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+1850 -974) |
Checks run (1 failed, 1 succeeded)
flake8
-
rbintegrations/slack/tests.py (Diff revision 4) line too long (81 > 79 characters) Column: 80 Error code: E501
-
rbintegrations/slack/tests.py (Diff revision 4) line too long (84 > 79 characters) Column: 80 Error code: E501
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+1858 -974) |
Checks run (2 succeeded)
Change Summary:
- Use "WebHook" consistently.
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+1874 -990) |
Checks run (2 succeeded)
-
-
rbintegrations/basechat/integration.py (Diff revision 6) This is an existing issue, but I noticed we have a space at the end and beginning of these strings, resulting in a double-space. Can you remove the leading space in the second string?
-
rbintegrations/basechat/integration.py (Diff revision 6) Can you pull this out into a local variable? We access it a lot, here and below.
-
rbintegrations/matrix/integration.py (Diff revision 6) Let's use
Sequence
instead oflist
, unless we need to mutate it. This solves some typing issues that can come up (probably not here, but in other cases).Same in other cases.
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+1912 -1008) |
Checks run (2 succeeded)
-
-
rbintegrations/slack/integration.py (Diff revisions 6 - 7) I'm curious, what happens if you try:
return JSONDict( attachments=[attachment], icon_url=integrations.logo_url )
Does it still complain?
Can I see the error?