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

Review Request #13749 — Created April 22, 2024 and submitted

Information

rbintegrations
release-4.x

Reviewers

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
Add typing and modernizations to chat integrations and allow using emoji unicode blocks in chats.
47dd9d235220cc648fc75332e98e33be3fb669be
Description From Last Updated

These are class properties, not constants, so they should be defined with lower case names. We should also type them …

daviddavid

The base class should probably use integration-assets/chat-common/

daviddavid

We've generally avoided python's ternary form. Can we just do if/else for these?

daviddavid

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

daviddavid

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

daviddavid

This is no longer correct.

chipx86chipx86

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

chipx86chipx86

Can we start moving this over to keyword-only arguments, and use housekeeping to deprecate? I think we're using the right …

chipx86chipx86

Same here.

chipx86chipx86

Same here, though we do have callers using review_request positionally. We can just make that positional and let everything else …

chipx86chipx86

line too long (81 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (84 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

This is an existing issue, but I noticed we have a space at the end and beginning of these strings, …

chipx86chipx86

Can you pull this out into a local variable? We access it a lot, here and below.

chipx86chipx86

Let's use Sequence instead of list, unless we need to mutate it. This solves some typing issues that can come …

chipx86chipx86

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 …

chipx86chipx86
david
  1. 
      
  2. rbintegrations/basechat/integration.py (Diff revision 1)
     
     
    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] = '...'
    
  3. rbintegrations/basechat/integration.py (Diff revision 1)
     
     
    Show all issues

    The base class should probably use integration-assets/chat-common/

  4. rbintegrations/basechat/integration.py (Diff revision 1)
     
     
     
    Show all issues

    We've generally avoided python's ternary form. Can we just do if/else for these?

  5. 
      
maubin
david
  1. 
      
  2. rbintegrations/basechat/integration.py (Diff revision 2)
     
     
    Show all issues

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

    1. 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.

    2. 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.

    3. Got it, will do!

  3. rbintegrations/slack/integration.py (Diff revision 2)
     
     
    Show all issues

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

  4. 
      
maubin
david
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
     
    Show all issues

    This is no longer correct.

  3. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
    Show all issues

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

    1. 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?

    2. 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.

  4. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
     
     
    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.

  5. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
     
     
    Show all issues

    Same here.

  6. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
     
     
    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.

    1. 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.

    2. Sounds good!

  7. 
      
maubin
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
david
  1. Ship It!
  2. 
      
maubin
david
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. rbintegrations/basechat/integration.py (Diff revision 6)
     
     
     
    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?

  3. rbintegrations/basechat/integration.py (Diff revision 6)
     
     
    Show all issues

    Can you pull this out into a local variable? We access it a lot, here and below.

  4. rbintegrations/matrix/integration.py (Diff revision 6)
     
     
    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.

  5. 
      
maubin
david
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. rbintegrations/slack/integration.py (Diff revisions 6 - 7)
     
     
     
     
     
     
     
    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?

    1. This is the original error:

      Image

      And when using JSONDict, I get this error and the original error remains:

      Image

    2. Okay, right, it's not safe to instantiate that..

      Go with the former, but you'll need to cast().

  3. 
      
maubin
chipx86
  1. Ship It!
  2. 
      
maubin
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.x (28a56e4)
Loading...