Summary:

Add some type hints for djblets.extensions.

Review Request #13074 — Created May 30, 2023 and submitted Aug. 2, 2023, 4:47 p.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
	Add some type hints for djblets.extensions.

Screenshots

Issues

Description	From	Last Updated
Typo in the description: "exetnsions"	chipx86	July 31, 2023, 4:15 p.m.
line too long (82 > 79 characters) Column: 80 Error code: E501	reviewbot	May 30, 2023, 7:49 p.m.
missing whitespace around parameter equals Column: 30 Error code: E252	reviewbot	May 30, 2023, 7:49 p.m.
'types.ModuleType' imported but unused Column: 1 Error code: F401	reviewbot	May 30, 2023, 7:49 p.m.
Swap these lines. Also, the typing imports aren't alphabetized (TYPE_CHECKING should be before Type).	chipx86	July 4, 2023, 4:37 p.m.
get_mod_func should be last.	chipx86	July 4, 2023, 4:37 p.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, 4:37 p.m.
We now have a JSONDict type we can use here.	chipx86	July 4, 2023, 4:38 p.m.
Same here.	chipx86	July 4, 2023, 4:38 p.m.
We repeat this a lot. Worth defining an ExtensionMetadata type?	chipx86	July 4, 2023, 4:47 p.m.
cast should be last (capitals are first).	chipx86	July 4, 2023, 4:38 p.m.
These should be swapped.	chipx86	July 4, 2023, 4:38 p.m.
**kwargs	chipx86	July 4, 2023, 4:38 p.m.
These are missing docs.	chipx86	July 4, 2023, 4:39 p.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, 4:40 p.m.
Can we set these types on the class? Same below.	chipx86	July 4, 2023, 4:44 p.m.
multiple spaces before operator Column: 33 Error code: E221	reviewbot	July 4, 2023, 4:53 p.m.
'typing.Type' imported but unused Column: 1 Error code: F401	reviewbot	July 4, 2023, 4:53 p.m.
'typing.TYPE_CHECKING' imported but unused Column: 1 Error code: F401	reviewbot	July 4, 2023, 4:53 p.m.
line too long (87 > 79 characters) Column: 80 Error code: E501	reviewbot	July 21, 2023, 7:42 p.m.
line too long (82 > 79 characters) Column: 80 Error code: E501	reviewbot	July 21, 2023, 7:42 p.m.
Can you add a from __future__ import annotations here?	chipx86	July 31, 2023, 4:15 p.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, 4:15 p.m.
This line is too long. Wrapping before Type should work.	chipx86	Aug. 2, 2023, 4:39 p.m.
Missing docs.	chipx86	July 31, 2023, 4:16 p.m.
This should probably be a JSONDict, since these will be stored in JSON.	chipx86	July 31, 2023, 4:16 p.m.
Missing docs.	chipx86	July 31, 2023, 4:17 p.m.
Missing Type:.	chipx86	Aug. 2, 2023, 4:40 p.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, 4:31 p.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, 4:25 p.m.
Should have Type:.	chipx86	July 31, 2023, 4:24 p.m.
Missing args and *kwargs.	chipx86	July 31, 2023, 4:23 p.m.
This can be super().	chipx86	July 31, 2023, 4:23 p.m.
Let's list public before private. Also, missing Type:.	chipx86	July 31, 2023, 4:22 p.m.
Why this removal?	chipx86	July 31, 2023, 4:22 p.m.
line too long (82 > 79 characters) Column: 80 Error code: E501	reviewbot	July 31, 2023, 4:36 p.m.

flake8 failed.

JSHint passed.

flake8

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

Show all issues

line too long (82 > 79 characters)

Column: 80
Error code: E501

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

Show all issues

missing whitespace around parameter equals

Column: 30
Error code: E252

djblets/extensions/manager.py (Diff revision 1)

Show all issues

'types.ModuleType' imported but unused

Column: 1
Error code: F401

Commits:

	Summary
-		Add some type hints for djblets.extensions.
+		Add some type hints for djblets.extensions.

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)

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)
Show all issues
```
get_mod_func should be last.
```

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.

djblets/extensions/extension.py (Diff revision 2)
Show all issues
```
We now have a JSONDict type we can use here.
```
djblets/extensions/extension.py (Diff revision 2)
Show all issues
```
Same here.
```

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

Show all issues

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

djblets/extensions/hooks.py (Diff revision 2)
Show all issues
```
cast should be last (capitals are first).
```
djblets/extensions/hooks.py (Diff revision 2)
Show all issues
```
These should be swapped.
```
djblets/extensions/hooks.py (Diff revision 2)
Show all issues
```
**kwargs
```
djblets/extensions/hooks.py (Diff revision 2)
Show all issues
```
These are missing docs.
```

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

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

Show all issues

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

Commits:

	Summary
-		Add some type hints for djblets.extensions.
+		Add some type hints for djblets.extensions.

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)

Show all issues

multiple spaces before operator

Column: 33
Error code: E221

djblets/extensions/packaging.py (Diff revision 3)

Show all issues

'typing.Type' imported but unused

Column: 1
Error code: F401

djblets/extensions/packaging.py (Diff revision 3)

Show all issues

'typing.TYPE_CHECKING' imported but unused

Column: 1
Error code: F401

Commits:

	Summary
-		Add some type hints for djblets.extensions.
+		Add some type hints for djblets.extensions.

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
-		Add some type hints for djblets.extensions.
+		Add some type hints for djblets.extensions.

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)

Show all issues

line too long (87 > 79 characters)

Column: 80
Error code: E501

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

Show all issues

line too long (82 > 79 characters)

Column: 80
Error code: E501

Commits:

	Summary
-		Add some type hints for djblets.extensions.
+		Add some type hints for djblets.extensions.

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.

Show all issues
```
Typo in the description: "exetnsions"
```

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

Show all issues

Can you add a from __future__ import annotations here?

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.

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

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

Show all issues

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

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

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.

david July 31, 2023, 4:32 p.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)

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, 4:32 p.m.

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

djblets/extensions/hooks.py (Diff revision 6)
Show all issues
```
Should have Type:.
```
djblets/extensions/hooks.py (Diff revision 6)
Show all issues
```
Missing *args and **kwargs.
```
djblets/extensions/hooks.py (Diff revision 6)
Show all issues
```
This can be super().
```

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

Show all issues

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

djblets/extensions/packaging.py (Diff revision 6)
Show all issues
```
Why this removal?
```
1. david July 31, 2023, 4:32 p.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
-		Add some type hints for djblets.extensions.
+		Add some type hints for djblets.extensions.

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)

Show all issues

line too long (82 > 79 characters)

Column: 80
Error code: E501

Ship it!

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

Status: Closed (submitted)

Change Summary:

Pushed to release-4.x (5728114)