Add some type hints for djblets.extensions.

Review Request #13074 — Created May 30, 2023 and submitted

Information

Djblets
release-4.x

Reviewers

This isn't a complete typing for the extensions module, but it's enough
to quash a lot of the warnings that show up when working on extensions
code.

Ran unit tests.

Summary ID
Add some type hints for djblets.extensions.
This isn't a complete typing for the extensions module, but it's enough to quash a lot of the warnings that show up when working on extensions code. Testing Done: Ran unit tests.
8a428af82d7691f210ddd061f54cfbbb97955860
Description From Last Updated

Typo in the description: "exetnsions"

chipx86chipx86

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

reviewbotreviewbot

missing whitespace around parameter equals Column: 30 Error code: E252

reviewbotreviewbot

'types.ModuleType' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

Swap these lines. Also, the typing imports aren't alphabetized (TYPE_CHECKING should be before Type).

chipx86chipx86

get_mod_func should be last.

chipx86chipx86

I think we can do one blank line before the TYPE_CHECKING block. Import-related conditionals are probably best considered as extra …

chipx86chipx86

We now have a JSONDict type we can use here.

chipx86chipx86

Same here.

chipx86chipx86

We repeat this a lot. Worth defining an ExtensionMetadata type?

chipx86chipx86

cast should be last (capitals are first).

chipx86chipx86

These should be swapped.

chipx86chipx86

**kwargs

chipx86chipx86

These are missing docs.

chipx86chipx86

While here, can we pull out registry so we don't re-fetch it every loop iteration? Same with __init__.

chipx86chipx86

Can we set these types on the class? Same below.

chipx86chipx86

multiple spaces before operator Column: 33 Error code: E221

reviewbotreviewbot

'typing.Type' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

'typing.TYPE_CHECKING' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Can you add a from __future__ import annotations here?

chipx86chipx86

These get so lengthy and are common enough, I think we can just continue to fit as many as needed …

chipx86chipx86

This line is too long. Wrapping before Type should work.

chipx86chipx86

Missing docs.

chipx86chipx86

This should probably be a JSONDict, since these will be stored in JSON.

chipx86chipx86

Missing docs.

chipx86chipx86

Missing Type:.

chipx86chipx86

Thinking about what to do here, and came across this: https://stackoverflow.com/a/69418048 I think Option 2 makes a fair amount of …

chipx86chipx86

Hmm wonder if we should just be storing this as a string when we assign it above. I think that …

chipx86chipx86

Should have Type:.

chipx86chipx86

Missing *args and **kwargs.

chipx86chipx86

This can be super().

chipx86chipx86

Let's list public before private. Also, missing Type:.

chipx86chipx86

Why this removal?

chipx86chipx86

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

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. This is a great change! Thanks for tackling this.

  2. djblets/extensions/extension.py (Diff revision 2)
     
     
     
    Show all issues

    Swap these lines.

    Also, the typing imports aren't alphabetized (TYPE_CHECKING should be before Type).

  3. djblets/extensions/extension.py (Diff revision 2)
     
     
    Show all issues

    get_mod_func should be last.

  4. djblets/extensions/extension.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    I think we can do one blank line before the TYPE_CHECKING block. Import-related conditionals are probably best considered as extra import groups.

  5. djblets/extensions/extension.py (Diff revision 2)
     
     
    Show all issues

    We now have a JSONDict type we can use here.

  6. djblets/extensions/extension.py (Diff revision 2)
     
     
    Show all issues

    Same here.

  7. djblets/extensions/extension.py (Diff revision 2)
     
     
    Show all issues

    We repeat this a lot. Worth defining an ExtensionMetadata type?

  8. djblets/extensions/hooks.py (Diff revision 2)
     
     
     
     
    Show all issues

    cast should be last (capitals are first).

  9. djblets/extensions/hooks.py (Diff revision 2)
     
     
     
    Show all issues

    These should be swapped.

  10. djblets/extensions/hooks.py (Diff revision 2)
     
     
    Show all issues

    **kwargs

  11. djblets/extensions/hooks.py (Diff revision 2)
     
     
     
     
     
     
    Show all issues

    These are missing docs.

  12. djblets/extensions/hooks.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    While here, can we pull out registry so we don't re-fetch it every loop iteration?

    Same with __init__.

  13. djblets/extensions/manager.py (Diff revision 2)
     
     
     
     
    Show all issues

    Can we set these types on the class? Same below.

  14. 
      
david
Review request changed
Commits:
Summary ID
Add some type hints for djblets.extensions.
This isn't a complete typing for the extensions module, but it's enough to quash a lot of the warnings that show up when working on exetnsions code. Testing Done: Ran unit tests.
ab040ecec4168e740da8aeec189b389051d4ff22
Add some type hints for djblets.extensions.
This isn't a complete typing for the extensions module, but it's enough to quash a lot of the warnings that show up when working on exetnsions code. Testing Done: Ran unit tests.
84aa5ece1cf759b90104759029bc282bea1e4000

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
david
Review request changed
Change Summary:

Merge with latest changes for pkg_resources

Commits:
Summary ID
Add some type hints for djblets.extensions.
This isn't a complete typing for the extensions module, but it's enough to quash a lot of the warnings that show up when working on exetnsions code. Testing Done: Ran unit tests.
24e0efc934880a86c8de65f2bdd1f58552c01c67
Add some type hints for djblets.extensions.
This isn't a complete typing for the extensions module, but it's enough to quash a lot of the warnings that show up when working on exetnsions code. Testing Done: Ran unit tests.
9cc989e6b5e4ba467d1b49503567a22371c8952e

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. Show all issues

    Typo in the description: "exetnsions"

  3. djblets/extensions/errors.py (Diff revision 6)
     
     
     
     
    Show all issues

    Can you add a from __future__ import annotations here?

  4. djblets/extensions/extension.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    These get so lengthy and are common enough, I think we can just continue to fit as many as needed on one line.

  5. djblets/extensions/extension.py (Diff revision 6)
     
     
    Show all issues

    Missing docs.

  6. djblets/extensions/extension.py (Diff revision 6)
     
     
    Show all issues

    This should probably be a JSONDict, since these will be stored in JSON.

  7. djblets/extensions/extension.py (Diff revision 6)
     
     
    Show all issues

    Missing docs.

  8. djblets/extensions/hooks.py (Diff revision 6)
     
     
    Show all issues

    Thinking about what to do here, and came across this:

    https://stackoverflow.com/a/69418048

    I think Option 2 makes a fair amount of sense in this particular case, since the signature is simple.

    1. No matter what I try, the cast version is the only thing I can get to work without complaints.

  9. djblets/extensions/hooks.py (Diff revision 6)
     
     
    Show all issues

    Hmm wonder if we should just be storing this as a string when we assign it above. I think that was the actual intent. Though that does technically break anything that might be using it as a UUID4.

    1. Yeah, I'd rather not mess with changing any types in API here.

  10. djblets/extensions/hooks.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
    Show all issues

    Should have Type:.

  11. djblets/extensions/hooks.py (Diff revision 6)
     
     
     
     
    Show all issues

    Missing *args and **kwargs.

  12. djblets/extensions/hooks.py (Diff revision 6)
     
     
    Show all issues

    This can be super().

  13. djblets/extensions/manager.py (Diff revision 6)
     
     
     
     
    Show all issues

    Let's list public before private.

    Also, missing Type:.

  14. djblets/extensions/packaging.py (Diff revision 6)
     
     
    Show all issues

    Why this removal?

    1. Just an error when merging with your big change to this file.

  15. 
      
david
Review request changed
Description:
   

This isn't a complete typing for the extensions module, but it's enough

~   to quash a lot of the warnings that show up when working on exetnsions
  ~ to quash a lot of the warnings that show up when working on extensions
    code.

Commits:
Summary ID
Add some type hints for djblets.extensions.
This isn't a complete typing for the extensions module, but it's enough to quash a lot of the warnings that show up when working on exetnsions code. Testing Done: Ran unit tests.
07aa2ae6bea336f22734d8258b8eaeb9ab32faff
Add some type hints for djblets.extensions.
This isn't a complete typing for the extensions module, but it's enough to quash a lot of the warnings that show up when working on extensions code. Testing Done: Ran unit tests.
8a428af82d7691f210ddd061f54cfbbb97955860

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

chipx86
  1. 
      
  2. djblets/extensions/extension.py (Diff revisions 6 - 7)
     
     
    Show all issues

    This line is too long. Wrapping before Type should work.

  3. djblets/extensions/extension.py (Diff revisions 6 - 7)
     
     
     
    Show all issues

    Missing Type:.

  4. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-4.x (5728114)