flake8
failed.
JSHint
passed.
flake8
-
reviewboard/reviews/views/mixins.py (Diff revision 1) Show all issues
Add type hints and modernize documentation for reviews/views/
Review Request #12685 — Created Oct. 17, 2022 and submitted
| Information | |
|---|---|
| david | |
| Review Board | |
| release-6.x | |
| Reviewers | |
| reviewboard | |
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).
Ran unit tests.
| Summary | |
|---|---|
| Description | From | Last Updated |
|---|---|---|
|
'typing.Dict' imported but unused Column: 1 Error code: F401 |
 reviewbot
|
|
|
Blank line between these. |
 chipx86
|
|
|
Missing Type: |
chipx86
|
|
|
SafeString should precede mark_safe here. |
chipx86
|
|
|
Blank line here. |
chipx86
|
|
|
These will need to specify the actual module path to link up right. |
chipx86
|
|
|
The description should be separated from the summary. |
chipx86
|
|
|
We should probably have a * in here somewhere to separate positional from keyword-only arguments, to keep this maintainable. There … |
chipx86
|
|
|
Same note about the module path. |
chipx86
|
|
|
This will also need to specify the module path. Wish we had some automatic review that could catch these. We'll … |
chipx86
|
|
|
line too long (82 > 79 characters) Column: 80 Error code: E501 |
reviewbot
|
|
|
unexpected spaces around keyword / parameter equals Column: 31 Error code: E251 |
reviewbot
|
|
|
Question: Since this is optional do we have to say datetime.datetime or None for the type? |
 maubin
|
Commits: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+1070 -320) |
|||||||||
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
Fantastic cleanup!
A few small things stood out, but nothing major.
-
-
-
reviewboard/reviews/views/bug_trackers.py (Diff revision 2) SafeStringshould precedemark_safehere. -
-
reviewboard/reviews/views/diff_fragments.py (Diff revision 2) These will need to specify the actual module path to link up right.
-
reviewboard/reviews/views/diff_fragments.py (Diff revision 2) The description should be separated from the summary.
-
reviewboard/reviews/views/diff_fragments.py (Diff revision 2) 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) 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?
Commits: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+1100 -332) |
|||||||||
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
-
reviewboard/notifications/email/message.py (Diff revision 3) line too long (82 > 79 characters) Column: 80 Error code: E501
-
reviewboard/notifications/email/message.py (Diff revision 3) unexpected spaces around keyword / parameter equals Column: 31 Error code: E251
Commits: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+1102 -332) |
|||||||||
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
reviewboard/reviews/views/mixins.py (Diff revision 4) Question: Since this is optional do we have to say
datetime.datetime or Nonefor the type?