Summary

Add type annotations to djblets.siteconfig.

Review Request #12702 — Created Oct. 28, 2022 and submitted Nov. 1, 2022, 11:08 p.m.

Information

Owner

chipx86

Repository

Djblets

Branch

release-3.x

Bugs

Depends On

Reviewers

Groups

djblets

People

Description

djblets.siteconfig, and SiteConfiguration in particular, is central
to much of the Djblets and Review Board codebases. This change
inrtoduces type annotations to most of the module, helping to make sure
call sites are passing in all the right information.

Settings management in SiteConfiguration is all typed now, helping
callers ensure they're handling results correctly.

Mappings between siteconfig and Django settings are typed with
structured TypeDicts, so we can avoid any issues with invalid keys or
values in those sometimes-complex mappings.

This was fairly smooth, though it did uncover a couple code paths where
assumptions were incorrectly made. Unbound varibable issues were fixed
in the management commands

Testing Done

All unit tests pass for all supported versions of Python.

Mypy and pyright were largely happy. There's some work in related modules
that still need to be done.

Commits

Summary	ID
Add type annotations to djblets.siteconfig. `djblets.siteconfig`, and `SiteConfiguration` in particular, is central to much of the Djblets and Review Board codebases. This change inrtoduces type annotations to most of the module, helping to make sure call sites are passing in all the right information. Settings management in `SiteConfiguration` is all typed now, helping callers ensure they're handling results correctly. Mappings between siteconfig and Django settings are typed with structured `TypeDict`s, so we can avoid any issues with invalid keys or values in those sometimes-complex mappings. This was fairly smooth, though it did uncover a couple code paths where assumptions were incorrectly made. Unbound varibable issues were fixed in the management commands	217011be690b2e2d947d3f2e616f0df16856c7f9

Summary

Add type annotations to djblets.siteconfig.

`djblets.siteconfig`, and `SiteConfiguration` in particular, is central to much of the Djblets and Review Board codebases. This change inrtoduces type annotations to most of the module, helping to make sure call sites are passing in all the right information. Settings management in `SiteConfiguration` is all typed now, helping callers ensure they're handling results correctly. Mappings between siteconfig and Django settings are typed with structured `TypeDict`s, so we can avoid any issues with invalid keys or values in those sometimes-complex mappings. This was fairly smooth, though it did uncover a couple code paths where assumptions were incorrectly made. Unbound varibable issues were fixed in the management commands

217011be690b2e2d947d3f2e616f0df16856c7f9

Issues

Description	From	Last Updated
'djblets.siteconfig.models.SiteConfigurationSettings' imported but unused Column: 1 Error code: F401	reviewbot	Oct. 28, 2022, 2:51 a.m.
'typing.Any' imported but unused Column: 1 Error code: F401	reviewbot	Oct. 28, 2022, 2:51 a.m.
Typo (I think?) "Djangos" -> "Django"	maubin	Nov. 1, 2022, 5:21 p.m.
Would it be worth it to include the type here like value: SiteConfigurationSettingsValue = siteconfig.get(key)?	maubin	Nov. 1, 2022, 5:22 p.m.
Typo: "are" -> "that are"	maubin	Nov. 1, 2022, 5:22 p.m.
Forgot to add str type.	maubin	Nov. 1, 2022, 5:22 p.m.
Could add unused here.	maubin	Nov. 1, 2022, 5:22 p.m.
Should add doc comments for these.	maubin	Nov. 1, 2022, 5:23 p.m.
Forgot to add str type.	maubin	Nov. 1, 2022, 5:23 p.m.
args should come after clear_caches. Also will need to add args to the doc string.	maubin	Nov. 1, 2022, 5:23 p.m.

flake8 failed.

JSHint passed.

flake8

djblets/siteconfig/forms.py (Diff revision 1)

Show all issues

'djblets.siteconfig.models.SiteConfigurationSettings' imported but unused

Column: 1
Error code: F401

djblets/siteconfig/models.py (Diff revision 1)

Show all issues

'typing.Any' imported but unused

Column: 1
Error code: F401

Change Summary:

Removed unused imports.

Commits:

	Summary	ID
	Add type annotations to djblets.siteconfig. `djblets.siteconfig`, and `SiteConfiguration` in particular, is central to much of the Djblets and Review Board codebases. This change inrtoduces type annotations to most of the module, helping to make sure call sites are passing in all the right information. Settings management in `SiteConfiguration` is all typed now, helping callers ensure they're handling results correctly. Mappings between siteconfig and Django settings are typed with structured `TypeDict`s, so we can avoid any issues with invalid keys or values in those sometimes-complex mappings. This was fairly smooth, though it did uncover a couple code paths where assumptions were incorrectly made. Unbound varibable issues were fixed in the management commands	92b47ddc5fbf9bfea949f2d45779a2afe5c77877
	Add type annotations to djblets.siteconfig. `djblets.siteconfig`, and `SiteConfiguration` in particular, is central to much of the Djblets and Review Board codebases. This change inrtoduces type annotations to most of the module, helping to make sure call sites are passing in all the right information. Settings management in `SiteConfiguration` is all typed now, helping callers ensure they're handling results correctly. Mappings between siteconfig and Django settings are typed with structured `TypeDict`s, so we can avoid any issues with invalid keys or values in those sometimes-complex mappings. This was fairly smooth, though it did uncover a couple code paths where assumptions were incorrectly made. Unbound varibable issues were fixed in the management commands	00ecd5102fc0093a1eca394e04a9da4d47502650

Diff:

Revision 2 (+592 -194)

Show changes

	djblets/db/fields/json_field.py
	djblets/siteconfig/django_settings.py
	djblets/siteconfig/forms.py
	djblets/siteconfig/managers.py
	djblets/siteconfig/middleware.py
	djblets/siteconfig/models.py
	djblets/siteconfig/views.py
	djblets/siteconfig/management/commands/get-siteconfig.py
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

djblets/siteconfig/django_settings.py (Diff revision 2)
Show all issues
```
Typo (I think?) "Djangos" -> "Django"
```
1. chipx86 Oct. 31, 2022, 2:02 p.m.
  All good catches, thanks!

djblets/siteconfig/django_settings.py (Diff revision 2)

Show all issues

Would it be worth it to include the type here like value: SiteConfigurationSettingsValue = siteconfig.get(key)?

chipx86 Oct. 31, 2022, 2:02 p.m.
```
That should be inferred through .get().
```

djblets/siteconfig/forms.py (Diff revision 2)
Show all issues
```
Typo: "are" -> "that are"
```
djblets/siteconfig/forms.py (Diff revision 2)
Show all issues
```
Forgot to add str type.
```
djblets/siteconfig/forms.py (Diff revision 2)
Show all issues
```
Could add unused here.
```

djblets/siteconfig/models.py (Diff revision 2)

Show all issues

Should add doc comments for these.

djblets/siteconfig/models.py (Diff revision 2)
Show all issues
```
Forgot to add str type.
```

djblets/siteconfig/models.py (Diff revision 2)

Show all issues

*args should come after clear_caches. Also will need to add *args to the doc string.

chipx86 Oct. 31, 2022, 2:02 p.m.

This is a weirdness with the new keyword-only argument syntax.
Python 3 allows arguments to be declared as keyword-only, so they won't be passed as positionals. These are listed after either a *args or a plain *. So these are both valid examples:

def func1(pos1, pos2, *, kwonly1=True, **kwargs):
    ...

def func2(post1, post2, *args, kwonly1=True, **kwargs):
    ...


I'll fix the docs.

maubin Oct. 31, 2022, 2:20 p.m.

Ah I see. I was only aware of the * way of doing that. Good to know.

chipx86 Nov. 1, 2022, 5:24 p.m.

Yeah it's not immediately obvious. And in fact, I put it right before **kwargs when I first wrote it, and had to go fix that.

Change Summary:


Fixed up a number of docstring issues, including typos and missing docs.
Added missing types.

Commits:

	Summary	ID
	Add type annotations to djblets.siteconfig. `djblets.siteconfig`, and `SiteConfiguration` in particular, is central to much of the Djblets and Review Board codebases. This change inrtoduces type annotations to most of the module, helping to make sure call sites are passing in all the right information. Settings management in `SiteConfiguration` is all typed now, helping callers ensure they're handling results correctly. Mappings between siteconfig and Django settings are typed with structured `TypeDict`s, so we can avoid any issues with invalid keys or values in those sometimes-complex mappings. This was fairly smooth, though it did uncover a couple code paths where assumptions were incorrectly made. Unbound varibable issues were fixed in the management commands	00ecd5102fc0093a1eca394e04a9da4d47502650
	Add type annotations to djblets.siteconfig. `djblets.siteconfig`, and `SiteConfiguration` in particular, is central to much of the Djblets and Review Board codebases. This change inrtoduces type annotations to most of the module, helping to make sure call sites are passing in all the right information. Settings management in `SiteConfiguration` is all typed now, helping callers ensure they're handling results correctly. Mappings between siteconfig and Django settings are typed with structured `TypeDict`s, so we can avoid any issues with invalid keys or values in those sometimes-complex mappings. This was fairly smooth, though it did uncover a couple code paths where assumptions were incorrectly made. Unbound varibable issues were fixed in the management commands	217011be690b2e2d947d3f2e616f0df16856c7f9

Diff:

Revision 3 (+612 -196)

Show changes

	djblets/db/fields/json_field.py
	djblets/siteconfig/django_settings.py
	djblets/siteconfig/forms.py
	djblets/siteconfig/managers.py
	djblets/siteconfig/middleware.py
	djblets/siteconfig/models.py
	djblets/siteconfig/views.py
	djblets/siteconfig/management/commands/get-siteconfig.py
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.x (264627b)