Summary

Add type hints and modernize documentation for reviews/views/

Review Request #12685 — Created Oct. 17, 2022 and submitted Nov. 17, 2022, 8:37 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-6.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This change adds type hints to the reviews.views module, and does some
updates to documentation (filling out a few missing docstrings and
editing others).

Testing Done

Ran unit tests.

Commits

Summary	ID
Add type hints and modernize documentation for reviews/views/ This change adds type hints to the reviews.views module, and does some updates to documentation (filling out a few missing docstrings and editing others). Testing Done: Ran unit tests.	6958c015cfaa8bc9f4dfa27bca4032536e31df4c

Issues

Description	From	Last Updated
'typing.Dict' imported but unused Column: 1 Error code: F401	reviewbot	Oct. 17, 2022, 9:33 a.m.
Blank line between these.	chipx86	Oct. 28, 2022, 12:41 p.m.
Missing Type:	chipx86	Oct. 28, 2022, 12:42 p.m.
SafeString should precede mark_safe here.	chipx86	Oct. 28, 2022, 12:42 p.m.
Blank line here.	chipx86	Oct. 28, 2022, 12:42 p.m.
These will need to specify the actual module path to link up right.	chipx86	Oct. 28, 2022, 12:43 p.m.
The description should be separated from the summary.	chipx86	Oct. 28, 2022, 12:43 p.m.
We should probably have a * in here somewhere to separate positional from keyword-only arguments, to keep this maintainable. There …	chipx86	Oct. 28, 2022, 1 p.m.
Same note about the module path.	chipx86	Oct. 28, 2022, 12:43 p.m.
This will also need to specify the module path. Wish we had some automatic review that could catch these. We'll …	chipx86	Oct. 28, 2022, 12:55 p.m.
line too long (82 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 28, 2022, 1:04 p.m.
unexpected spaces around keyword / parameter equals Column: 31 Error code: E251	reviewbot	Oct. 28, 2022, 1:04 p.m.
Question: Since this is optional do we have to say datetime.datetime or None for the type?	maubin	Oct. 28, 2022, 2:30 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/views/mixins.py (Diff revision 1)
The issue has been dropped. Show all issues
```
'typing.Dict' imported but unused

Column: 1
Error code: F401
```

Commits:

	Summary	ID
	Add type hints and modernize documentation for reviews/views/ This change adds type hints to the reviews.views module, and does some updates to documentation (filling out a few missing docstrings and editing others). Testing Done: Ran unit tests.	6047279af99c4d04713af3cc3b89f22a2a416e5f
	Add type hints and modernize documentation for reviews/views/ This change adds type hints to the reviews.views module, and does some updates to documentation (filling out a few missing docstrings and editing others). Testing Done: Ran unit tests.	12b65c4843d5ffc111cb061efe7ad2e20758d750

Diff:

Revision 2 (+1070 -320)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/notifications/email/views.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/tests/test_review_request_view_mixin.py
	reviewboard/reviews/views/attachments.py
	reviewboard/reviews/views/bug_trackers.py
	reviewboard/reviews/views/diff_fragments.py
	reviewboard/reviews/views/diffviewer.py
	8 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Fantastic cleanup!

A few small things stood out, but nothing major.

reviewboard/reviews/models/review_request.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between these.
```
reviewboard/reviews/models/review_request.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing Type:
```
reviewboard/reviews/views/bug_trackers.py (Diff revision 2)
The issue has been resolved. Show all issues
```
SafeString should precede mark_safe here.
```
reviewboard/reviews/views/diff_fragments.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line here.
```
reviewboard/reviews/views/diff_fragments.py (Diff revision 2)
The issue has been resolved. Show all issues
```
These will need to specify the actual module path to link up right.
```
reviewboard/reviews/views/diff_fragments.py (Diff revision 2)
The issue has been resolved. Show all issues
```
The description should be separated from the summary.
```

reviewboard/reviews/views/diff_fragments.py (Diff revision 2)

The issue has been resolved. Show all issues

We should probably have a * in here somewhere to separate positional from keyword-only arguments, to keep this maintainable. There might be other similar methods that could benefit.

reviewboard/reviews/views/diff_fragments.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Same note about the module path.
```

reviewboard/reviews/views/diff_fragments.py (Diff revision 2)

The issue has been resolved. Show all issues

This will also need to specify the module path.

Wish we had some automatic review that could catch these. We'll have to someday get automatic doc building as part of the tests.

Can you build docs and see if anything comes up for any of these?

david Oct. 28, 2022, 1:01 p.m.

Unfortunately I don't see any warning output.

Commits:

	Summary	ID
	Add type hints and modernize documentation for reviews/views/ This change adds type hints to the reviews.views module, and does some updates to documentation (filling out a few missing docstrings and editing others). Testing Done: Ran unit tests.	12b65c4843d5ffc111cb061efe7ad2e20758d750
	Add type hints and modernize documentation for reviews/views/ This change adds type hints to the reviews.views module, and does some updates to documentation (filling out a few missing docstrings and editing others). Testing Done: Ran unit tests.	3db5468075a577ca44bbf7984cb911927054dfda

Diff:

Revision 3 (+1100 -332)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/views.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/tests/test_review_request_view_mixin.py
	reviewboard/reviews/views/attachments.py
	reviewboard/reviews/views/bug_trackers.py
	reviewboard/reviews/views/diff_fragments.py
	9 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/notifications/email/message.py (Diff revision 3)
The issue has been dropped. Show all issues
```
line too long (82 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/notifications/email/message.py (Diff revision 3)
The issue has been dropped. Show all issues
```
unexpected spaces around keyword / parameter equals

Column: 31
Error code: E251
```

Commits:

	Summary	ID
	Add type hints and modernize documentation for reviews/views/ This change adds type hints to the reviews.views module, and does some updates to documentation (filling out a few missing docstrings and editing others). Testing Done: Ran unit tests.	3db5468075a577ca44bbf7984cb911927054dfda
	Add type hints and modernize documentation for reviews/views/ This change adds type hints to the reviews.views module, and does some updates to documentation (filling out a few missing docstrings and editing others). Testing Done: Ran unit tests.	6958c015cfaa8bc9f4dfa27bca4032536e31df4c

Diff:

Revision 4 (+1102 -332)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/views.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/tests/test_review_request_view_mixin.py
	reviewboard/reviews/views/attachments.py
	reviewboard/reviews/views/bug_trackers.py
	reviewboard/reviews/views/diff_fragments.py
	9 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/views/mixins.py (Diff revision 4)

The issue has been dropped. Show all issues

Question: Since this is optional do we have to say datetime.datetime or None for the type?

david Oct. 28, 2022, 2:30 p.m.

We probably could, but I think we can probably look at that at a case-by-case basis. This is pretty obscure API that we're probably rarely going to touch ever, much less have a third party use it.

chipx86 Oct. 28, 2022, 2:34 p.m.

We generally don't say or None in the type (though we could, but historically we haven't, and type annotations can now convey that a bit better). Instead, it's more helpful to describe the condition under which a None result might be returned instead.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-6.x (d39b58f)