Summary

Add typing structures for the diff viewer context.

Review Request #13666 — Created March 25, 2024 and submitted April 12, 2024, 12:39 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

The only actual change to the content of the context is that we can no
longer end up with non-bool types for the is_draft_diff and
is_draft_interdiff fields.

Testing Done

Ran unit tests.

Commits

Summary	ID
Add typing structures for the diff viewer context. The context that's assembled and used to render the diff viewer page (both the HTML-based UI and the diff context API endpoint) has grown organically since the very earliest days of Review Board, and is a pretty big hairy mess. In order to bring at least a little bit of sanity to it, this change adds typing structures so that type checkers can verify that we're always assigning things correctly. The only actual change to the content of the context is that we can no longer end up with non-`bool` types for the `is_draft_diff` and `is_draft_interdiff` fields. Testing Done: - Ran unit tests.	eaf2f91109549ee9bfe835fbac9bc42b86f397aa

Summary

Add typing structures for the diff viewer context.

The context that's assembled and used to render the diff viewer page (both the HTML-based UI and the diff context API endpoint) has grown organically since the very earliest days of Review Board, and is a pretty big hairy mess. In order to bring at least a little bit of sanity to it, this change adds typing structures so that type checkers can verify that we're always assigning things correctly. The only actual change to the content of the context is that we can no longer end up with non-`bool` types for the `is_draft_diff` and `is_draft_interdiff` fields. Testing Done: - Ran unit tests.

eaf2f91109549ee9bfe835fbac9bc42b86f397aa

Issues

Description	From	Last Updated
Missing Returns.	chipx86	April 2, 2024, 8:36 a.m.
Might be a good time to deprecate non-keyword-only arguments.	chipx86	April 2, 2024, 8:39 a.m.
Can you add Args for these?	chipx86	April 2, 2024, 8:40 a.m.
If we're not documenting these, we don't need blank lines between them.	chipx86	April 2, 2024, 8:41 a.m.
Blank line between these.	chipx86	April 2, 2024, 8:42 a.m.
These can be assertFalse.	chipx86	April 2, 2024, 8:42 a.m.

flake8 passed.

JSHint passed.

Overall looks good, but I have two broad thoughts:

Discussion point: I wonder if we should consider putting at least some of these serialized types into a module that can own them. This wouldn't make sense everywhere, but might help keep some things organized? It'd be a fine line, so I don't really have anything more to offer on this.
I think we probably should document these serialized types, if they're intended to be consumed outside of the module. I know we talked about this in the context of TypeScript, consuming APIs, but when we're providing them in a situation where changes may impact a caller's use, we should document them so there's a clear path to versioning and deprecating.

david April 2, 2024, 9:23 a.m.


Hmm. I'll have to think about this. For now I think keeping those definitions close to the views that use them is best.
Adding docs isn't too much of a burden, though the amount that we "use" these is pretty limited outside of the code that's changed in here, plus the templates that render the pages.

reviewboard/diffviewer/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing Returns.
```
reviewboard/reviews/context.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Might be a good time to deprecate non-keyword-only arguments.
```

reviewboard/reviews/context.py (Diff revision 1)

The issue has been resolved. Show all issues

Can you add Args for these?

reviewboard/reviews/models/review_request.py (Diff revision 1)
The issue has been resolved. Show all issues
```
If we're not documenting these, we don't need blank lines between them.
```
reviewboard/reviews/views/diffviewer.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line between these.
```
reviewboard/webapi/tests/test_diff_context.py (Diff revision 1)
The issue has been resolved. Show all issues
```
These can be assertFalse.
```

Commits:

	Summary	ID
	Add typing structures for the diff viewer context. The context that's assembled and used to render the diff viewer page (both the HTML-based UI and the diff context API endpoint) has grown organically since the very earliest days of Review Board, and is a pretty big hairy mess. In order to bring at least a little bit of sanity to it, this change adds typing structures so that type checkers can verify that we're always assigning things correctly. The only actual change to the content of the context is that we can no longer end up with non-`bool` types for the `is_draft_diff` and `is_draft_interdiff` fields. Testing Done: - Ran unit tests.	03f29610d7496efc3344e9c8a65d8f07acc4285c
	Add typing structures for the diff viewer context. The context that's assembled and used to render the diff viewer page (both the HTML-based UI and the diff context API endpoint) has grown organically since the very earliest days of Review Board, and is a pretty big hairy mess. In order to bring at least a little bit of sanity to it, this change adds typing structures so that type checkers can verify that we're always assigning things correctly. The only actual change to the content of the context is that we can no longer end up with non-`bool` types for the `is_draft_diff` and `is_draft_interdiff` fields. Testing Done: - Ran unit tests.	eaf2f91109549ee9bfe835fbac9bc42b86f397aa

Diff:

Revision 2 (+1398 -216)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/models/diffcommit.py
	reviewboard/reviews/context.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/ui/base.py
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-7.x (73c3882)