Summary

Add some type hints for djblets.extensions.

Review Request #13074 — Created May 30, 2023 and submitted Aug. 2, 2023, 9:47 a.m.

Information

Owner

david

Repository

Djblets

Branch

release-4.x

Bugs

Depends On

Reviewers

Groups

djblets

People

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 extensions
code.

Testing Done

Ran unit tests.

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 extensions code. Testing Done: Ran unit tests.	8a428af82d7691f210ddd061f54cfbbb97955860

Issues

Description	From	Last Updated
Typo in the description: "exetnsions"	chipx86	July 31, 2023, 9:15 a.m.
line too long (82 > 79 characters) Column: 80 Error code: E501	reviewbot	May 30, 2023, 12:49 p.m.
missing whitespace around parameter equals Column: 30 Error code: E252	reviewbot	May 30, 2023, 12:49 p.m.
'types.ModuleType' imported but unused Column: 1 Error code: F401	reviewbot	May 30, 2023, 12:49 p.m.
Swap these lines. Also, the typing imports aren't alphabetized (TYPE_CHECKING should be before Type).	chipx86	July 4, 2023, 9:37 a.m.
get_mod_func should be last.	chipx86	July 4, 2023, 9:37 a.m.
I think we can do one blank line before the TYPE_CHECKING block. Import-related conditionals are probably best considered as extra …	chipx86	July 4, 2023, 9:37 a.m.
We now have a JSONDict type we can use here.	chipx86	July 4, 2023, 9:38 a.m.
Same here.	chipx86	July 4, 2023, 9:38 a.m.
We repeat this a lot. Worth defining an ExtensionMetadata type?	chipx86	July 4, 2023, 9:47 a.m.
cast should be last (capitals are first).	chipx86	July 4, 2023, 9:38 a.m.
These should be swapped.	chipx86	July 4, 2023, 9:38 a.m.
**kwargs	chipx86	July 4, 2023, 9:38 a.m.
These are missing docs.	chipx86	July 4, 2023, 9:39 a.m.
While here, can we pull out registry so we don't re-fetch it every loop iteration? Same with __init__.	chipx86	July 4, 2023, 9:40 a.m.
Can we set these types on the class? Same below.	chipx86	July 4, 2023, 9:44 a.m.
multiple spaces before operator Column: 33 Error code: E221	reviewbot	July 4, 2023, 9:53 a.m.
'typing.Type' imported but unused Column: 1 Error code: F401	reviewbot	July 4, 2023, 9:53 a.m.
'typing.TYPE_CHECKING' imported but unused Column: 1 Error code: F401	reviewbot	July 4, 2023, 9:53 a.m.
line too long (87 > 79 characters) Column: 80 Error code: E501	reviewbot	July 21, 2023, 12:42 p.m.
line too long (82 > 79 characters) Column: 80 Error code: E501	reviewbot	July 21, 2023, 12:42 p.m.
Can you add a from __future__ import annotations here?	chipx86	July 31, 2023, 9:15 a.m.
These get so lengthy and are common enough, I think we can just continue to fit as many as needed …	chipx86	July 31, 2023, 9:15 a.m.
This line is too long. Wrapping before Type should work.	chipx86	Aug. 2, 2023, 9:39 a.m.
Missing docs.	chipx86	July 31, 2023, 9:16 a.m.
This should probably be a JSONDict, since these will be stored in JSON.	chipx86	July 31, 2023, 9:16 a.m.
Missing docs.	chipx86	July 31, 2023, 9:17 a.m.
Missing Type:.	chipx86	Aug. 2, 2023, 9:40 a.m.
Thinking about what to do here, and came across this: https://stackoverflow.com/a/69418048 I think Option 2 makes a fair amount of …	chipx86	July 31, 2023, 9:31 a.m.
Hmm wonder if we should just be storing this as a string when we assign it above. I think that …	chipx86	July 31, 2023, 9:25 a.m.
Should have Type:.	chipx86	July 31, 2023, 9:24 a.m.
Missing args and *kwargs.	chipx86	July 31, 2023, 9:23 a.m.
This can be super().	chipx86	July 31, 2023, 9:23 a.m.
Let's list public before private. Also, missing Type:.	chipx86	July 31, 2023, 9:22 a.m.
Why this removal?	chipx86	July 31, 2023, 9:22 a.m.
line too long (82 > 79 characters) Column: 80 Error code: E501	reviewbot	July 31, 2023, 9:36 a.m.

flake8 failed.

JSHint passed.

flake8

djblets/extensions/extension.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line too long (82 > 79 characters)

Column: 80
Error code: E501
```
djblets/extensions/hooks.py (Diff revision 1)
The issue has been dropped. Show all issues
```
missing whitespace around parameter equals

Column: 30
Error code: E252
```
djblets/extensions/manager.py (Diff revision 1)
The issue has been dropped. Show all issues
```
'types.ModuleType' imported but unused

Column: 1
Error code: F401
```

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.	035171d8f2e6f3a9a38fb608513cb69a4116e1dd
	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

Diff:

Revision 2 (+1228 -438)

Show changes

	djblets/extensions/errors.py
	djblets/extensions/extension.py
	djblets/extensions/hooks.py
	djblets/extensions/manager.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

This is a great change! Thanks for tackling this.

djblets/extensions/extension.py (Diff revision 2)

The issue has been resolved. Show all issues

Swap these lines.

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

djblets/extensions/extension.py (Diff revision 2)
The issue has been resolved. Show all issues
```
get_mod_func should be last.
```

djblets/extensions/extension.py (Diff revision 2)

The issue has been resolved. 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.

djblets/extensions/extension.py (Diff revision 2)
The issue has been resolved. Show all issues
```
We now have a JSONDict type we can use here.
```
djblets/extensions/extension.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Same here.
```
djblets/extensions/extension.py (Diff revision 2)
The issue has been resolved. Show all issues
```
We repeat this a lot. Worth defining an ExtensionMetadata type?
```
djblets/extensions/hooks.py (Diff revision 2)
The issue has been resolved. Show all issues
```
cast should be last (capitals are first).
```
djblets/extensions/hooks.py (Diff revision 2)
The issue has been resolved. Show all issues
```
These should be swapped.
```
djblets/extensions/hooks.py (Diff revision 2)
The issue has been resolved. Show all issues
```
**kwargs
```
djblets/extensions/hooks.py (Diff revision 2)
The issue has been resolved. Show all issues
```
These are missing docs.
```

djblets/extensions/hooks.py (Diff revision 2)

The issue has been resolved. Show all issues

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

Same with __init__.

djblets/extensions/manager.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we set these types on the class? Same below.
```

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

Diff:

Revision 3 (+1290 -446)

Show changes

	djblets/extensions/errors.py
	djblets/extensions/extension.py
	djblets/extensions/hooks.py
	djblets/extensions/manager.py
	djblets/extensions/packaging.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

djblets/extensions/manager.py (Diff revision 3)
The issue has been dropped. Show all issues
```
multiple spaces before operator

Column: 33
Error code: E221
```
djblets/extensions/packaging.py (Diff revision 3)
The issue has been dropped. Show all issues
```
'typing.Type' imported but unused

Column: 1
Error code: F401
```
djblets/extensions/packaging.py (Diff revision 3)
The issue has been dropped. Show all issues
```
'typing.TYPE_CHECKING' imported but unused

Column: 1
Error code: F401
```

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.	84aa5ece1cf759b90104759029bc282bea1e4000
	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

Diff:

Revision 4 (+1288 -444)

Show changes

	djblets/extensions/errors.py
	djblets/extensions/extension.py
	djblets/extensions/hooks.py
	djblets/extensions/manager.py
	djblets/extensions/packaging.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

Diff:

Revision 5 (+1190 -384)

Show changes

	djblets/extensions/errors.py
	djblets/extensions/extension.py
	djblets/extensions/hooks.py
	djblets/extensions/manager.py
	djblets/extensions/packaging.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

djblets/extensions/extension.py (Diff revision 5)
The issue has been dropped. Show all issues
```
line too long (87 > 79 characters)

Column: 80
Error code: E501
```
djblets/extensions/extension.py (Diff revision 5)
The issue has been dropped. Show all issues
```
line too long (82 > 79 characters)

Column: 80
Error code: E501
```

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.	9cc989e6b5e4ba467d1b49503567a22371c8952e
	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

Diff:

Revision 6 (+1214 -384)

Show changes

	djblets/extensions/errors.py
	djblets/extensions/extension.py
	djblets/extensions/hooks.py
	djblets/extensions/manager.py
	djblets/extensions/packaging.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues
```
Typo in the description: "exetnsions"
```
djblets/extensions/errors.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Can you add a from __future__ import annotations here?
```

djblets/extensions/extension.py (Diff revision 6)

The issue has been resolved. 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.

djblets/extensions/extension.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Missing docs.
```
djblets/extensions/extension.py (Diff revision 6)
The issue has been resolved. Show all issues
```
This should probably be a JSONDict, since these will be stored in JSON.
```
djblets/extensions/extension.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Missing docs.
```

djblets/extensions/hooks.py (Diff revision 6)

The issue has been dropped. 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.

david July 31, 2023, 9:32 a.m.

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

djblets/extensions/hooks.py (Diff revision 6)

The issue has been dropped. 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.

david July 31, 2023, 9:32 a.m.

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

djblets/extensions/hooks.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Should have Type:.
```
djblets/extensions/hooks.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Missing *args and **kwargs.
```
djblets/extensions/hooks.py (Diff revision 6)
The issue has been resolved. Show all issues
```
This can be super().
```
djblets/extensions/manager.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Let's list public before private.
Also, missing Type:.
```
djblets/extensions/packaging.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Why this removal?
```
1. david July 31, 2023, 9:32 a.m.
  Just an error when merging with your big change to this file.

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

Diff:

Revision 7 (+1244 -384)

Show changes

	djblets/extensions/errors.py
	djblets/extensions/extension.py
	djblets/extensions/hooks.py
	djblets/extensions/manager.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

djblets/extensions/extension.py (Diff revision 7)
The issue has been resolved. Show all issues
```
line too long (82 > 79 characters)

Column: 80
Error code: E501
```

Ship it!

djblets/extensions/extension.py (Diff revisions 6 - 7)
The issue has been resolved. Show all issues
```
This line is too long. Wrapping before Type should work.
```
djblets/extensions/extension.py (Diff revisions 6 - 7)
The issue has been resolved. Show all issues
```
Missing Type:.
```

Status: Closed (submitted)

Change Summary:

Pushed to release-4.x (5728114)