flake8
-
reviewboard/reviews/managers.py (Diff revision 1) Show all issues
Review Request #12885 — Created March 14, 2023 and submitted
Information | |
---|---|
david | |
Review Board | |
release-6.x | |
Reviewers | |
reviewboard | |
This change removes all RemovedInReviewBoard60 things from our codebase.
The only complicated part of this is that the mercurial diff parsers
were still using the oldget_orig_commit_id
method, so those have been
updated to set the data correctly on theParsedDiffChange
.This also makes one small fix to the filediff API resource to handle a
deprecation removal in Djblets.
Ran unit tests.
Summary | |
---|---|
Description | From | Last Updated |
---|---|---|
Can you verify if rbintegrations, Power Pack, and Review Bot tests pass? |
|
|
I think we need to add this here: * Made ``diff_settings`` mandatory. |
![]() |
|
Same comment here and below about adding the * Made ``diff_settings`` mandatory. |
![]() |
|
I think in Christian's review he mentioned that these should all be keyword only, and to add the decorator. |
![]() |
|
line too long (82 > 79 characters) Column: 80 Error code: E501 |
![]() |
|
While here, can you fix up the ordering of these imported classes? |
|
|
Given the "diff_settings will be required," and diff_settings being keyword-only, we can remove the None and callers will have to … |
|
|
This could also be removed once the above is changed. |
|
|
We don't explicitly state this (oops), but as the old argument is deprecated, we can remove enable_syntax_highlighting here and probably … |
|
|
The docs state that parsed_diff_change is mandatory in 6.0. Since this is a keyword argument, I propose: Making all arguments … |
|
|
While here and impacting keyword argument order, let's make all these keyword-only arguments and use the decorator to deprecate positional … |
|
|
This should also be removed. |
|
|
Since we're affecting keyword argument order now, let's make these keyword-only and add the decorator. |
|
|
Same comment re: keyword-only arguments and decorator. |
|
|
line too long (81 > 79 characters) Column: 80 Error code: E501 |
![]() |
|
line too long (85 > 79 characters) Column: 80 Error code: E501 |
![]() |
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+140 -2364) |
Summary: |
|
|||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||
Commits: |
|
|||||||||||||||||||||
Diff: |
Revision 3 (+142 -2366) |
The change as-is looks great.
Many of the changing function signatures should also be made keyword-only while we're changing them. I commented on the major locations. This could all be done separately from this change though.
reviewboard/admin/widgets.py (Diff revision 3) |
---|
While here, can you fix up the ordering of these imported classes?
reviewboard/diffviewer/chunk_generator.py (Diff revision 3) |
---|
Given the "diff_settings will be required," and
diff_settings
being keyword-only, we can remove theNone
and callers will have to provide it (will need a re-run of the test suite though).
reviewboard/diffviewer/chunk_generator.py (Diff revision 3) |
---|
This could also be removed once the above is changed.
reviewboard/diffviewer/diffutils.py (Diff revision 3) |
---|
We don't explicitly state this (oops), but as the old argument is deprecated, we can remove
enable_syntax_highlighting
here and probably makediff_settings
mandatory.
reviewboard/diffviewer/parser.py (Diff revision 3) |
---|
The docs state that
parsed_diff_change
is mandatory in 6.0. Since this is a keyword argument, I propose:
- Making all arguments keyword-only, and documenting that.
- Use the
@deprecate_non_keyword_only_args
arguments.- Assert
parsed_diff_change
until 7.That will also let us remove the conditional below.
reviewboard/diffviewer/renderers.py (Diff revision 3) |
---|
While here and impacting keyword argument order, let's make all these keyword-only arguments and use the decorator to deprecate positional use.
As per docs,
diff_settings
is also now required, so we can remove theNone
.
reviewboard/reviews/managers.py (Diff revision 3) |
---|
Since we're affecting keyword argument order now, let's make these keyword-only and add the decorator.
reviewboard/scmtools/managers.py (Diff revision 3) |
---|
Same comment re: keyword-only arguments and decorator.
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+532 -2616) |
reviewboard/deprecation.py (Diff revision 4) |
---|
line too long (81 > 79 characters) Column: 80 Error code: E501
reviewboard/diffviewer/tests/test_diff_chunk_generator.py (Diff revision 4) |
---|
line too long (85 > 79 characters) Column: 80 Error code: E501
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+542 -2622) |
reviewboard/diffviewer/chunk_generator.py (Diff revisions 1 - 5) |
---|
I think we need to add this here:
* Made ``diff_settings`` mandatory.
reviewboard/diffviewer/diffutils.py (Diff revisions 1 - 5) |
---|
Same comment here and below about adding the
* Made ``diff_settings`` mandatory.
reviewboard/diffviewer/renderers.py (Diff revisions 1 - 5) |
---|
I think in Christian's review he mentioned that these should all be keyword only, and to add the decorator.
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+548 -2620) |