flake8
-
reviewboard/reviews/ui/diff.py (Diff revision 1) -
reviewboard/reviews/views/diffviewer.py (Diff revision 1) dictionary key 'index' repeated with different values Column: 17 Error code: F601
Review Request #13661 — Created March 21, 2024 and submitted
The diff comment serialization worked very differently from file
attachment and screenshot comment serialization. This change updates
that to match.The structure used to be:
[ { "linenum": X, "num_lines": Y, "comments": [ ... comments ... ] }, ... ]
Now it's:
{ "X-Y": [ ... comments ... ] }
The object keys in the new format are used to group all comments on the
same block together, but are otherwise opaque. Instead, like we do for
file attachments, we'll pick theline
andnum_lines
values out of
the first comment in the list.It's arguable that the diff format was better than the one used for file
attachments, since the line number and number of lines for each block
really should be serialized separately from the individual comments.
Unfortunately, the file attachment format isn't easily changeable because
extensions that add Review UIs exist in the wild, and we need to
maintain compatibility there.
Summary | ID |
---|---|
7929db2ad40d313998b201468cfb0748f4c4a370 |
Description | From | Last Updated |
---|---|---|
undefined name 'DiffSet' Column: 14 Error code: F821 |
reviewbot | |
dictionary key 'index' repeated with different values Column: 17 Error code: F601 |
reviewbot | |
These don't support generics on Python 3.8. |
chipx86 | |
Version Added? |
chipx86 | |
This should use :file:. |
chipx86 | |
Version Added? |
chipx86 | |
These should be documented. |
chipx86 | |
This needs to be the full module path. |
chipx86 | |
These are missing docs. |
chipx86 | |
This isn't specific to this change, but while I'm here... I think the parent class should take generics for the … |
chipx86 | |
Looks like we're missing typing for this (unless it's in a base class?) |
chipx86 | |
Is this covered by the previous typing changes? |
chipx86 | |
Can we work toward keyword-only arguments for these constructors? |
chipx86 | |
Should DiffReviewUI just take these as keyword arguments? |
chipx86 | |
Should be :file:. |
chipx86 | |
Should probably be import type. |
chipx86 | |
Missing docs. |
chipx86 | |
Missing docs. |
chipx86 | |
Do we not get these types from the serialized data? |
chipx86 |
reviewboard/reviews/ui/diff.py (Diff revision 1) |
---|
reviewboard/reviews/views/diffviewer.py (Diff revision 1) |
---|
dictionary key 'index' repeated with different values Column: 17 Error code: F601
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+656 -488) |
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+650 -488) |
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+648 -488) |
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+656 -514) |
reviewboard/reviews/ui/diff.py (Diff revision 5) |
---|
This isn't specific to this change, but while I'm here...
I think the parent class should take generics for the comment type and serialized comment type. That way there's no need to cast everywhere, and callers can always get the right typing when requesting comment types, when accepting a type of the base class.
reviewboard/reviews/views/diffviewer.py (Diff revision 5) |
---|
Is this covered by the previous typing changes?
reviewboard/reviews/views/diffviewer.py (Diff revision 5) |
---|
Can we work toward keyword-only arguments for these constructors?
reviewboard/reviews/views/diffviewer.py (Diff revision 5) |
---|
Should
DiffReviewUI
just take these as keyword arguments?
reviewboard/static/rb/js/reviews/models/diffFileModel.ts (Diff revision 5) |
---|
Should probably be
import type
.
reviewboard/static/rb/js/reviews/models/diffReviewableModel.ts (Diff revision 5) |
---|
Do we not get these types from the serialized data?
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+716 -516) |
reviewboard/reviews/ui/diff.py (Diff revisions 5 - 6) |
---|
Looks like we're missing typing for this (unless it's in a base class?)
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+728 -518) |
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+734 -518) |