Summary

Add typing to reviews/detail.py

Review Request #14019 — Created July 9, 2024 and submitted Aug. 5, 2024, 9:08 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This change adds type hints throughout our ReviewRequestPageData and
entry implementations.

Testing Done

Ran unit tests.

Commits

Summary	ID
Add typing to reviews/detail.py This change adds type hints throughout our ReviewRequestPageData and entry implementations. Testing Done: Ran unit tests.	f8b33c4d5e73098ec88af02b242d6f4b4297f155

Issues

Description	From	Last Updated
too many blank lines (3) Column: 1 Error code: E303	reviewbot	July 9, 2024, 4:48 p.m.
The attributes section should be removed.	maubin	July 17, 2024, 7:26 a.m.
Can you add , optional here.	maubin	July 17, 2024, 7:27 a.m.
Can you add , optional here.	maubin	July 17, 2024, 7:27 a.m.
When taking things in, we should ideally use Sequence instead of list (unless we're assigning directly to a list).	chipx86	July 31, 2024, 9:57 a.m.
We should return a Mapping with a Sequence instead, so it's not mutable.	chipx86	July 31, 2024, 9:57 a.m.
Same here: Sequence and Mapping.	chipx86	July 31, 2024, 9:57 a.m.
I think Iterator is correct here, since the function yields items and is therefore an iterator, rather than returning something …	chipx86	July 31, 2024, 10:04 a.m.
We should maybe begin moving to keyword-only arguments. Note that subclasses would need any deprecation decorators applied to their versions …	chipx86	July 31, 2024, 9:58 a.m.
I feel like it'd be preferable to keep this up above where the other attributes are, and keep functions below …	chipx86	July 31, 2024, 9:58 a.m.
Summary must be on its own line.	chipx86	July 31, 2024, 9:59 a.m.
Summary must be on its own line.	chipx86	July 31, 2024, 9:59 a.m.
It'd be nice to move to keyword-only arguments here.	chipx86	July 31, 2024, 9:59 a.m.
It'd be nice to move to keyword-only arguments here. Same caveat as above.	chipx86	July 31, 2024, 9:59 a.m.
Should be a django.utils.safestring.SafeString.	chipx86	July 31, 2024, 9:59 a.m.
We should use Sequence or maybe Iterable here.	chipx86	July 31, 2024, 10 a.m.
Can we type this?	chipx86	July 31, 2024, 10 a.m.
Should maybe be Iterable.	chipx86	July 31, 2024, 10:01 a.m.
Here, too.	chipx86	July 31, 2024, 10:01 a.m.
Missing docs.	chipx86	July 31, 2024, 10:01 a.m.
This needs to be Type, given Python 3.8 compatibility.	chipx86	July 31, 2024, 10:01 a.m.
Should be Iterable.	chipx86	July 31, 2024, 10:02 a.m.
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	July 31, 2024, 10:06 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/detail.py (Diff revision 1)
The issue has been dropped. Show all issues
```
too many blank lines (3)

Column: 1
Error code: E303
```

Commits:

	Summary	ID
	Add typing to reviews/detail.py This change adds type hints throughout our ReviewRequestPageData and entry implementations. Testing Done: Ran unit tests.	93061085709e43fa2a147fdd90b43b2aef597e5f
	Add typing to reviews/detail.py This change adds type hints throughout our ReviewRequestPageData and entry implementations. Testing Done: Ran unit tests.	a5785e9f5c54c47f0574bd7e89233d359536a34e

Diff:

Revision 2 (+658 -400)

Show changes

reviewboard/reviews/detail.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/detail.py (Diff revision 2)

The issue has been resolved. Show all issues

The attributes section should be removed.

reviewboard/reviews/detail.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you add , optional here.
```
reviewboard/reviews/detail.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you add , optional here.
```

Commits:

	Summary	ID
	Add typing to reviews/detail.py This change adds type hints throughout our ReviewRequestPageData and entry implementations. Testing Done: Ran unit tests.	a5785e9f5c54c47f0574bd7e89233d359536a34e
	Add typing to reviews/detail.py This change adds type hints throughout our ReviewRequestPageData and entry implementations. Testing Done: Ran unit tests.	46d0af1834f6d1b8baa631856f0edd845f71ae6e

Diff:

Revision 3 (+660 -424)

Show changes

reviewboard/reviews/detail.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

reviewboard/reviews/detail.py (Diff revision 3)

The issue has been resolved. Show all issues

When taking things in, we should ideally use Sequence instead of list (unless we're assigning directly to a list).

reviewboard/reviews/detail.py (Diff revision 3)
The issue has been resolved. Show all issues
```
We should return a Mapping with a Sequence instead, so it's not mutable.
```
reviewboard/reviews/detail.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Same here: Sequence and Mapping.
```

reviewboard/reviews/detail.py (Diff revision 3)

The issue has been dropped. Show all issues

I think Iterator is correct here, since the function yields items and is therefore an iterator, rather than returning something that can be iterated over.
Usually Iterator is taken in as something you want to iterate over, and Iterator (a subtype of Iterator) is returned as a thing from a function providing iteration.

david July 31, 2024, 10:05 a.m.

Type checking gets annoyed with that if the function (like in this case) doesn't actually yield anything.

reviewboard/reviews/detail.py (Diff revision 3)

The issue has been dropped. Show all issues

We should maybe begin moving to keyword-only arguments.

Note that subclasses would need any deprecation decorators applied to their versions as well. Same with any other cases like this.

chipx86 July 31, 2024, 4:02 a.m.

Feel free to just put these aside for now, and track for another change.

reviewboard/reviews/detail.py (Diff revision 3)

The issue has been resolved. Show all issues

I feel like it'd be preferable to keep this up above where the other attributes are, and keep functions below any attributes, even if they're classmethods. These get heavily buried otherwise.

reviewboard/reviews/detail.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Summary must be on its own line.
```
reviewboard/reviews/detail.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Summary must be on its own line.
```
reviewboard/reviews/detail.py (Diff revision 3)
The issue has been dropped. Show all issues
```
It'd be nice to move to keyword-only arguments here.
```
reviewboard/reviews/detail.py (Diff revision 3)
The issue has been dropped. Show all issues
```
It'd be nice to move to keyword-only arguments here.
Same caveat as above.
```
reviewboard/reviews/detail.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Should be a django.utils.safestring.SafeString.
```
reviewboard/reviews/detail.py (Diff revision 3)
The issue has been resolved. Show all issues
```
We should use Sequence or maybe Iterable here.
```
reviewboard/reviews/detail.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we type this?
```
reviewboard/reviews/detail.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Should maybe be Iterable.
```
reviewboard/reviews/detail.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Here, too.
```
reviewboard/reviews/detail.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing docs.
```
reviewboard/reviews/detail.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This needs to be Type, given Python 3.8 compatibility.
```
reviewboard/reviews/detail.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Should be Iterable.
```

Commits:

	Summary	ID
	Add typing to reviews/detail.py This change adds type hints throughout our ReviewRequestPageData and entry implementations. Testing Done: Ran unit tests.	46d0af1834f6d1b8baa631856f0edd845f71ae6e
	Add typing to reviews/detail.py This change adds type hints throughout our ReviewRequestPageData and entry implementations. Testing Done: Ran unit tests.	5e627ae8571b43d2d0ccd4fae4b16e6917d5e49c

Diff:

Revision 4 (+670 -426)

Show changes

reviewboard/reviews/detail.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/detail.py (Diff revision 4)
The issue has been dropped. Show all issues
```
line too long (80 > 79 characters)

Column: 80
Error code: E501
```

Commits:

	Summary	ID
	Add typing to reviews/detail.py This change adds type hints throughout our ReviewRequestPageData and entry implementations. Testing Done: Ran unit tests.	5e627ae8571b43d2d0ccd4fae4b16e6917d5e49c
	Add typing to reviews/detail.py This change adds type hints throughout our ReviewRequestPageData and entry implementations. Testing Done: Ran unit tests.	f8b33c4d5e73098ec88af02b242d6f4b4297f155

Diff:

Revision 5 (+674 -426)

Show changes

reviewboard/reviews/detail.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-7.x (0d4c04f)