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

 






 

 


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

 




















 

  

   Information

  





 
 

  maubin
 









 
 

  rbintegrations
 









 
 

  release-4.x
 









 
 

  
 









 
 

  
 









 
 

  13742
 






 




 

  

   Reviewers

  





 
 

  rbintegrations
 









 
 

  
 






 








 





  


 
 

  
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




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  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. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       rbintegrations/basechat/integration.py
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



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

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  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. 




  5. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  

 
















 



 

  

   




maubin




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  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. 



    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. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      Got it, will do!
      
 
      

      4. 


 
    


    

 
    

    3. 






  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. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 



 

  

   




maubin




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       rbintegrations/basechat/integration.py
       


        (Diff revision 3)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This is no longer correct.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  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. 



    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.
      
 
      

      3. 


 
    


    

 
    

    4. 






  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. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       rbintegrations/basechat/integration.py
       


        (Diff revision 3)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Same here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 






  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. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Sounds good!
      
 
      

      3. 


 
    


    

 
    

    7. 




  7. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 




  

 
















 



 

  

   




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

 


 















  

 
















 



 

  

   




maubin




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




maubin




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  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. 






  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. 






  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. 




  5. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  

 
















 



 

  

   




maubin




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  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. 



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

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

      3. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




maubin




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




maubin




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  







  

   

    Status:
   

   


    Completed

   

  




  

   

    Change Summary:
   

   

    
Pushed to release-4.x (28a56e4)