Summary

Add type hints to ReviewUI implementation.

Review Request #13514 — Created Jan. 31, 2024 and submitted Feb. 12, 2024, 9:53 a.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This change adds type hints to the Python side of the Review UI
implementation.

While I was in here, I also discovered that the screenshot Review UI was
calling into Screenshot.display_id, which didn't exist. This code path
is effectively dead because the vast majority of installs don't use the
Screenshot model at all, but it could theoretically cause crashes on
very old review requests that do.

Testing Done

Ran unit tests.

Commits

Summary	ID
Add type hints to ReviewUI implementation. This change adds type hints to the Python side of the Review UI implementation. While I was in here, I also discovered that the screenshot Review UI was calling into Screenshot.display_id, which didn't exist. This code path is effectively dead because the vast majority of installs don't use the Screenshot model at all, but it could theoretically cause crashes on very old review requests that do. Testing Done: Ran unit tests.	c4cb2a5e89693f440bf793ecc22289c29a9a302e

Summary

Add type hints to ReviewUI implementation.

This change adds type hints to the Python side of the Review UI implementation. While I was in here, I also discovered that the screenshot Review UI was calling into Screenshot.display_id, which didn't exist. This code path is effectively dead because the vast majority of installs don't use the Screenshot model at all, but it could theoretically cause crashes on very old review requests that do. Testing Done: Ran unit tests.

c4cb2a5e89693f440bf793ecc22289c29a9a302e

Issues

Description	From	Last Updated
do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()` Column: 16 Error …	reviewbot	Jan. 31, 2024, 2:22 p.m.
'typing.TYPE_CHECKING' imported but unused Column: 1 Error code: F401	reviewbot	Jan. 31, 2024, 2:22 p.m.
Could we use a List of djblets.util.typing.JSONDict here since the return value must be serializable into JSON?	maubin	Feb. 2, 2024, 2:08 p.m.
Same here and below for other serialize methods, can we use djblets.util.typing.JSONDict?	maubin	Feb. 2, 2024, 2:08 p.m.
Doesn't this return django.utils.safestring.SafeText instead of a normal string?	maubin	Feb. 2, 2024, 2:11 p.m.
Can use JSONDict here.	maubin	Feb. 5, 2024, 8:30 a.m.
Can use JSONDict here. Also missing docs.	maubin	Feb. 5, 2024, 8:30 a.m.
Can use JSONDict here.	maubin	Feb. 5, 2024, 8:32 a.m.
Can use JSONDict here.	maubin	Feb. 5, 2024, 8:33 a.m.
Can use JSONDict here.	maubin	Feb. 5, 2024, 8:33 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/ui/base.py (Diff revision 1)

The issue has been dropped. Show all issues

do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`

Column: 16
Error code: E721

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

Column: 1
Error code: F401
```

Commits:

	Summary	ID
	Add type hints to ReviewUI implementation. This change adds type hints to the Python side of the Review UI implementation. While I was in here, I also discovered that the screenshot Review UI was calling into Screenshot.display_id, which didn't exist. This code path is effectively dead because the vast majority of installs don't use the Screenshot model at all, but it could theoretically cause crashes on very old review requests that do. Testing Done: Ran unit tests.	314f71f36cf9688abd833a912c646c48dcaff0d9
	Add type hints to ReviewUI implementation. This change adds type hints to the Python side of the Review UI implementation. While I was in here, I also discovered that the screenshot Review UI was calling into Screenshot.display_id, which didn't exist. This code path is effectively dead because the vast majority of installs don't use the Screenshot model at all, but it could theoretically cause crashes on very old review requests that do. Testing Done: Ran unit tests.	481d2061277e92f9e0d944770c9910968ec322f2

Diff:

Revision 2 (+1046 -218)

Show changes

	reviewboard/reviews/models/screenshot.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/ui/image.py
	reviewboard/reviews/ui/markdownui.py
	reviewboard/reviews/ui/screenshot.py
	reviewboard/reviews/ui/text.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/ui/base.py (Diff revision 2)

The issue has been resolved. Show all issues

Could we use a List of djblets.util.typing.JSONDict here since the return value must be serializable into JSON?

reviewboard/reviews/ui/base.py (Diff revision 2)

The issue has been resolved. Show all issues

Same here and below for other serialize methods, can we use djblets.util.typing.JSONDict?

reviewboard/reviews/ui/text.py (Diff revision 2)

The issue has been dropped. Show all issues

Doesn't this return django.utils.safestring.SafeText instead of a normal string?

david Feb. 2, 2024, 2:11 p.m.

render_to_string returns str. In the template we use {{thumbnail|default:''|safe}} to mark it as safe.

Commits:

	Summary	ID
	Add type hints to ReviewUI implementation. This change adds type hints to the Python side of the Review UI implementation. While I was in here, I also discovered that the screenshot Review UI was calling into Screenshot.display_id, which didn't exist. This code path is effectively dead because the vast majority of installs don't use the Screenshot model at all, but it could theoretically cause crashes on very old review requests that do. Testing Done: Ran unit tests.	481d2061277e92f9e0d944770c9910968ec322f2
	Add type hints to ReviewUI implementation. This change adds type hints to the Python side of the Review UI implementation. While I was in here, I also discovered that the screenshot Review UI was calling into Screenshot.display_id, which didn't exist. This code path is effectively dead because the vast majority of installs don't use the Screenshot model at all, but it could theoretically cause crashes on very old review requests that do. Testing Done: Ran unit tests.	893ab915813937555fc2c60e0bd411879767476d

Diff:

Revision 3 (+1048 -218)

Show changes

	reviewboard/reviews/models/screenshot.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/ui/image.py
	reviewboard/reviews/ui/markdownui.py
	reviewboard/reviews/ui/screenshot.py
	reviewboard/reviews/ui/text.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/ui/base.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can use JSONDict here.
```
reviewboard/reviews/ui/image.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can use JSONDict here. Also missing docs.
```
reviewboard/reviews/ui/image.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can use JSONDict here.
```
reviewboard/reviews/ui/screenshot.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can use JSONDict here.
```
reviewboard/reviews/ui/text.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can use JSONDict here.
```

Commits:

	Summary	ID
	Add type hints to ReviewUI implementation. This change adds type hints to the Python side of the Review UI implementation. While I was in here, I also discovered that the screenshot Review UI was calling into Screenshot.display_id, which didn't exist. This code path is effectively dead because the vast majority of installs don't use the Screenshot model at all, but it could theoretically cause crashes on very old review requests that do. Testing Done: Ran unit tests.	893ab915813937555fc2c60e0bd411879767476d
	Add type hints to ReviewUI implementation. This change adds type hints to the Python side of the Review UI implementation. While I was in here, I also discovered that the screenshot Review UI was calling into Screenshot.display_id, which didn't exist. This code path is effectively dead because the vast majority of installs don't use the Screenshot model at all, but it could theoretically cause crashes on very old review requests that do. Testing Done: Ran unit tests.	c4cb2a5e89693f440bf793ecc22289c29a9a302e

Diff:

Revision 4 (+1076 -218)

Show changes

	reviewboard/reviews/models/screenshot.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/ui/image.py
	reviewboard/reviews/ui/markdownui.py
	reviewboard/reviews/ui/screenshot.py
	reviewboard/reviews/ui/text.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed