Summary

Add typing/modernizations to BaseChatIntegration and allow using emoji unicode blocks in chats.

Review Request #13749 — Created April 22, 2024 and updated May 3, 2024, 6:20 p.m.

Information

Owner

maubin

Repository

rbintegrations

Branch

release-4.x

Bugs

Depends On

Blocks

13742

Reviewers

Groups

rbintegrations

People

Description

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.

Testing Done

Ran unit tests.

Commits

Summary	ID
Add typing and modernizations to chat integrations and allow using emoji unicode blocks in chats.	43fe547f6061a1be30f0129470e901197fae7026

Issues

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	April 23, 2024, 9:04 a.m.
The base class should probably use integration-assets/chat-common/	david	April 23, 2024, 9:05 a.m.
We've generally avoided python's ternary form. Can we just do if/else for these?	david	April 23, 2024, 9:11 a.m.
We can use type instead of Type. This should probably also be ClassVar[type[...]]	david	April 23, 2024, 12:04 p.m.
Now that we changed the base class, this probably needs to override assets_base_url to point to the slack subdir.	david	April 23, 2024, 11:19 a.m.
This is no longer correct.	chipx86	April 23, 2024, 9:27 p.m.
Can we make this a set? That'll be cheaper to look up file extensions in.	chipx86	April 23, 2024, 3:37 p.m.
Can we start moving this over to keyword-only arguments, and use housekeeping to deprecate? I think we're using the right …	chipx86	April 24, 2024, 9:36 a.m.
Same here.	chipx86	April 24, 2024, 9:37 a.m.
Same here, though we do have callers using review_request positionally. We can just make that positional and let everything else …	chipx86	April 24, 2024, 12:03 p.m.
line too long (81 > 79 characters) Column: 80 Error code: E501	reviewbot	April 24, 2024, 4:13 p.m.
line too long (84 > 79 characters) Column: 80 Error code: E501	reviewbot	April 24, 2024, 4:13 p.m.
This is an existing issue, but I noticed we have a space at the end and beginning of these strings, …	chipx86	April 30, 2024, 7:45 a.m.
Can you pull this out into a local variable? We access it a lot, here and below.	chipx86	April 30, 2024, 7:50 a.m.
Let's use Sequence instead of list, unless we need to mutate it. This solves some typing issues that can come …	chipx86	April 30, 2024, 8:11 a.m.
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	May 3, 2024, 6:20 p.m.

flake8 passed.

JSHint passed.

rbintegrations/basechat/integration.py (Diff revision 1)

The issue has been resolved. Show all issues

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 issue has been resolved. Show all issues
```
The base class should probably use integration-assets/chat-common/
```
rbintegrations/basechat/integration.py (Diff revision 1)
The issue has been resolved. Show all issues
```
We've generally avoided python's ternary form. Can we just do if/else for these?
```

Commits:

	Summary	ID
	Add typing and modernizations to BaseChatIntegration and allow using emoji unicode blocks in chats.	3790ad8b948122faa793f7a899742e8ecaaa8216
	Add typing and modernizations to BaseChatIntegration and allow using emoji unicode blocks in chats.	99daf99bb3cf4a226d309d7330370fcd452e038a

Diff:

Revision 2 (+828 -606)

Show changes

	rbintegrations/basechat/integration.py
	rbintegrations/discord/integration.py
	rbintegrations/discord/tests.py
	rbintegrations/matrix/integration.py
	rbintegrations/mattermost/integration.py
	rbintegrations/mattermost/tests.py
	rbintegrations/slack/integration.py
	rbintegrations/slack/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbintegrations/basechat/integration.py (Diff revision 2)

The issue has been resolved. Show all issues

We can use type instead of Type. This should probably also be ClassVar[type[...]]

maubin April 23, 2024, 11:18 a.m.

On the Djblets Integration class, we don't use ClassVar, we just do config_form_cls: Type[IntegrationConfigForm]. So if I use ClassVar here mypy complains about overriding an instance variable with a class one. I could make a change in Djblets where I fix up the typing and add  ClassVar to the Integration class. Or I could just leave out the ClassVar here, which is not correct but not the worst :P. Let me know what you think I should do.

david April 23, 2024, 11:21 a.m.

I think fixing up the type in djblets is a great idea. I suspect we'll hit a lot of cases where our early implementation of type hints isn't 100% correct and we have to go back to fix them up.

maubin April 23, 2024, 11:31 a.m.
```
Got it, will do!
```

rbintegrations/slack/integration.py (Diff revision 2)

The issue has been resolved. Show all issues

Now that we changed the base class, this probably needs to override assets_base_url to point to the slack subdir.

Commits:

	Summary	ID
	Add typing and modernizations to BaseChatIntegration and allow using emoji unicode blocks in chats.	99daf99bb3cf4a226d309d7330370fcd452e038a
	Add typing and modernizations to BaseChatIntegration and allow using emoji unicode blocks in chats.	7a398c35998ced328712f5a1d3bd3f1950080487

Diff:

Revision 3 (+834 -604)

Show changes

	rbintegrations/basechat/integration.py
	rbintegrations/discord/integration.py
	rbintegrations/discord/tests.py
	rbintegrations/matrix/integration.py
	rbintegrations/mattermost/integration.py
	rbintegrations/mattermost/tests.py
	rbintegrations/slack/integration.py
	rbintegrations/slack/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

rbintegrations/basechat/integration.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This is no longer correct.
```

rbintegrations/basechat/integration.py (Diff revision 3)

The issue has been dropped. Show all issues

Can we make this a set? That'll be cheaper to look up file extensions in.

maubin April 23, 2024, 3:30 p.m.

I was thinking about doing this, but was thrown off by the way we currently check the file extensions. We do filename.endswith(self.valid_image_url_exts), where endswith() only works with tuples (or strings), not sets. I thought it would be cheaper to just keep this instead of having to split out the extension from the filename, and then do a set lookup.

After researching this, I see that endswith() has a time complexity of O(n) on average. Set operations are usually O(1) so that's fine, but for os.path.splitext(), the implementation depends on the operating system so I'm not really sure what the time complexity would be. I'm guessing it's usually something like O(n)? In that case I still think that just sticking with the tuple and doing endsWith() is cheaper, what do you think?

chipx86 April 23, 2024, 3:37 p.m.

Yeah you're probably right. Let's keep it as-is. If we don't need to split the filename anyway, there's no point in doing so, as well.

rbintegrations/basechat/integration.py (Diff revision 3)

The issue has been resolved. Show all issues

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)
The issue has been resolved. Show all issues
```
Same here.
```

rbintegrations/basechat/integration.py (Diff revision 3)

The issue has been resolved. Show all issues

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.

maubin April 23, 2024, 3:30 p.m.

I'll probably do this in a separate change where I add housekeeping and a new deprecation error for rbintegrations. But that shouldn't take too long at all so once I do that I'll update this change to use keyword-only arguments.

chipx86 April 23, 2024, 3:37 p.m.
```
Sounds good!
```

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:

~		This change mainly adds typing and modernizations to the BaseChatIntegration
~		class, and cleans up some duplicate code in its subclasses.
	~	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.

Commits:

	Summary	ID
	Add typing and modernizations to BaseChatIntegration and allow using emoji unicode blocks in chats.	7a398c35998ced328712f5a1d3bd3f1950080487
	Add typing and modernizations to chat integrations and allow using emoji unicode blocks in chats.	0b0961f777f6c297bbbaaa1cb9a56c706ca63a92

Diff:

Revision 4 (+1850 -974)

Show changes

	rbintegrations/basechat/integration.py
	rbintegrations/discord/integration.py
	rbintegrations/discord/tests.py
	rbintegrations/matrix/integration.py
	rbintegrations/mattermost/integration.py
	rbintegrations/mattermost/tests.py
	rbintegrations/slack/integration.py
	rbintegrations/slack/tests.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbintegrations/slack/tests.py (Diff revision 4)
The issue has been resolved. Show all issues
```
line too long (81 > 79 characters)

Column: 80
Error code: E501
```
rbintegrations/slack/tests.py (Diff revision 4)
The issue has been resolved. Show all issues
```
line too long (84 > 79 characters)

Column: 80
Error code: E501
```

Commits:

	Summary	ID
	Add typing and modernizations to chat integrations and allow using emoji unicode blocks in chats.	0b0961f777f6c297bbbaaa1cb9a56c706ca63a92
	Add typing and modernizations to chat integrations and allow using emoji unicode blocks in chats.	8287cc38c6d7d6e660f93004d5938460e49fe6f2

Diff:

Revision 5 (+1858 -974)

Show changes

	rbintegrations/basechat/integration.py
	rbintegrations/discord/integration.py
	rbintegrations/discord/tests.py
	rbintegrations/matrix/integration.py
	rbintegrations/mattermost/integration.py
	rbintegrations/mattermost/tests.py
	rbintegrations/slack/integration.py
	rbintegrations/slack/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Change Summary:


Use "WebHook" consistently.

Commits:

	Summary	ID
	Add typing and modernizations to chat integrations and allow using emoji unicode blocks in chats.	8287cc38c6d7d6e660f93004d5938460e49fe6f2
	Add typing and modernizations to chat integrations and allow using emoji unicode blocks in chats.	b8043da23577ec5c2ee5fe661fb7cb73e01dd076

Diff:

Revision 6 (+1874 -990)

Show changes

	rbintegrations/basechat/integration.py
	rbintegrations/discord/integration.py
	rbintegrations/discord/tests.py
	rbintegrations/matrix/integration.py
	rbintegrations/mattermost/integration.py
	rbintegrations/mattermost/tests.py
	rbintegrations/slack/integration.py
	rbintegrations/slack/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

rbintegrations/basechat/integration.py (Diff revision 6)

The issue has been resolved. Show all issues

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)
The issue has been resolved. Show all issues
```
Can you pull this out into a local variable? We access it a lot, here and below.
```

rbintegrations/matrix/integration.py (Diff revision 6)

The issue has been resolved. Show all issues

Let's use Sequence instead of list, 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:

	Summary	ID
	Add typing and modernizations to chat integrations and allow using emoji unicode blocks in chats.	b8043da23577ec5c2ee5fe661fb7cb73e01dd076
	Add typing and modernizations to chat integrations and allow using emoji unicode blocks in chats.	43fe547f6061a1be30f0129470e901197fae7026

Diff:

Revision 7 (+1912 -1008)

Show changes

	rbintegrations/basechat/integration.py
	rbintegrations/discord/integration.py
	rbintegrations/discord/tests.py
	rbintegrations/matrix/integration.py
	rbintegrations/mattermost/integration.py
	rbintegrations/mattermost/tests.py
	rbintegrations/slack/integration.py
	rbintegrations/slack/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Fix it!

rbintegrations/slack/integration.py (Diff revisions 6 - 7)

An issue was opened. Show all issues

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?