flake8
-
reviewboard/diffviewer/tests/test_diffcommit_manager.py (Diff revision 1) Show all issues
Review Request #9631 — Created Feb. 12, 2018 and submitted
The main logic of the
DiffSetManager
has been refactored into two
parts:
reviewboard.diffviewer.filediff_creator
, which handles theFileDiff
s; andDiffManagerBase
, which provides a base implementation ofcreate_from_upload
for both the DiffSetManager
and theDiffCommitManager
.The new DiffCommitManager
is able to create DiffCommit
objects from
either uploaded files or from data, just as the DiffSetManager
can for
DiffSet
objects.
This patch also adds DiffSetManager.create_empty
to create empty
DiffSets which will contain DiffCommits
.
Ran unit tests.
Description | From | Last Updated |
---|---|---|
F401 'nose' imported but unused |
![]() |
|
typo: reivewboard |
|
|
Missing a period. |
|
|
) should be on the line before. |
|
|
Docstring? |
|
|
There are so many args here, let's put one per line. |
|
|
Can we put one arg per line? |
|
|
) should be on the previous line and no trailing comma after revision=1 |
|
|
F841 local variable 'history' is assigned to but never used |
![]() |
|
W391 blank line at end of file |
![]() |
|
While here, can you make request default to None and be optional? It's used for an eventual call to get_file_exists(), … |
|
|
"will be empty" |
|
|
Can you pass these as keyword arguments? It'll be easier for maintenance. |
|
|
"filenames" |
|
|
"filename" |
|
|
"filename" |
|
|
"filenames" |
|
|
Can you fix the indentation while you're here? |
|
|
Missing a docstring. |
|
|
Missing a docstring. |
|
|
This read better the way it was before. |
|
|
It's not valid to put more text here between sections. |
|
|
Can you revert this? It doesn't simplify the code any, and makes it less future-proof. Better to have one task … |
|
|
Like above, can we make this optional? |
|
|
Is this necessary, given that this is a new manager for a new model? If it is, we should document … |
|
|
Can you pass a keyword argument here, so it's self-describing? |
|
|
It seems that DiffSetManager.create_from_data() and DiffCommitManager.create_from_data() do primarily the same thing, with the exception that the latter creates and passes … |
|
|
Can you use a keyword argument here? |
|
|
Keyword arguments would also be great here. |
|
|
This is the manager for DiffSet. I'm not sure DiffCommit is right here? |
|
|
F841 local variable 'filediffs' is assigned to but never used |
![]() |
|
Other such functions say "Subclasses." Would be nice to be consistent. |
|
|
E126 continuation line over-indented for hanging indent |
![]() |
|
Swap these. |
|
|
Should be "BaseDiffManager", to match other base classes. |
|
|
Missing an entry for **kwargs. |
|
|
I think instead of copying all the keyword arguments from create_from_data, we just document **kwargs and say these are passed … |
|
|
No u prefix. Also missing docs for this argument. |
|
|
Missing docs for this argument. |
|
|
Missing a period. |
|
|
This says optional, but the argument doesn't have a default. |
|
|
This is in the wrong spot with regards to the argument list. |
|
|
Why don't we pass basedir anymore? |
|
|
Missing docs for **kwargs. |
|
|
"F" before "P". |
|
|
, optional |
|
|
Missing backtick and period. |
|
|
The ( should be on the next line. Kind of looks weird after another paren though. |
|
|
Blank line between these. |
|
|
F401 'reviewboard.diffviewer.features.dvcs_feature' imported but unused |
![]() |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+696 -192) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+725 -194) |
reviewboard/diffviewer/tests/test_diffset_manager.py (Diff revision 3) |
---|
F841 local variable 'history' is assigned to but never used
reviewboard/diffviewer/tests/test_diffset_manager.py (Diff revision 3) |
---|
W391 blank line at end of file
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+723 -194) |
reviewboard/diffviewer/managers.py (Diff revision 2) |
---|
There are so many args here, let's put one per line.
reviewboard/diffviewer/tests/test_diffcommit_manager.py (Diff revision 2) |
---|
) should be on the previous line and no trailing comma after
revision=1
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+751 -198) |
reviewboard/diffviewer/managers.py (Diff revision 5) |
---|
While here, can you make
request
default toNone
and be optional? It's used for an eventual call toget_file_exists()
, which doesn't require it. That will make it easier for testing work (I've hit this before while going through some complex diff testing for interdiffs).
reviewboard/diffviewer/managers.py (Diff revision 5) |
---|
Can you pass these as keyword arguments? It'll be easier for maintenance.
reviewboard/diffviewer/managers.py (Diff revision 5) |
---|
It's not valid to put more text here between sections.
reviewboard/diffviewer/managers.py (Diff revision 5) |
---|
Can you revert this? It doesn't simplify the code any, and makes it less future-proof. Better to have one task per line here.
reviewboard/diffviewer/managers.py (Diff revision 5) |
---|
Is this necessary, given that this is a new manager for a new model?
If it is, we should document why in a comment here, and this should specify the right function and class name.
reviewboard/diffviewer/managers.py (Diff revision 5) |
---|
Can you pass a keyword argument here, so it's self-describing?
reviewboard/diffviewer/managers.py (Diff revision 5) |
---|
It seems that
DiffSetManager.create_from_data()
andDiffCommitManager.create_from_data()
do primarily the same thing, with the exception that the latter creates and passes aDiffCommit
. It'd be nice to consolidate as much of this functionality as possible. I imagine the base class could have its own implementation thatDiffCommitManager
could override, calling the parent with adiff_commit=
argument.
reviewboard/diffviewer/managers.py (Diff revision 5) |
---|
This is the manager for
DiffSet
. I'm not sureDiffCommit
is right here?
Addressed Christian's issues.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+757 -201) |
reviewboard/diffviewer/managers.py (Diff revision 6) |
---|
F841 local variable 'filediffs' is assigned to but never used
Lint.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+757 -201) |
Fixed unit test failure.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+756 -201) |
reviewboard/diffviewer/managers.py (Diff revision 8) |
---|
Just realized this is going to be confusing with the other processor classes that are coming (for chunk/opcode processing). Maybe
FileDiffsCreator
.It'd also be nice to move this to its own class, since this file's getting big otherwise.
reviewboard/diffviewer/managers.py (Diff revision 8) |
---|
Other such functions say "Subclasses." Would be nice to be consistent.
Refactor + address issues.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 9 (+801 -211) |
Rebase
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+801 -211) |
reviewboard/diffviewer/filediff_creator.py (Diff revision 10) |
---|
E126 continuation line over-indented for hanging indent
Fixups
Description: |
|
|||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 11 (+800 -211) |
Do not pass basedir and base_commit_id to DiffCommitManager.create_from_data. fixes unit test failures in next patch
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 12 (+808 -221) |
reviewboard/diffviewer/managers.py (Diff revision 12) |
---|
Should be "BaseDiffManager", to match other base classes.
reviewboard/diffviewer/managers.py (Diff revision 12) |
---|
I think instead of copying all the keyword arguments from
create_from_data
, we just document**kwargs
and say these are passed tocreate_from_data
. Makes it easier to maintain and prevents discrepencies.
reviewboard/diffviewer/managers.py (Diff revision 12) |
---|
No
u
prefix.Also missing docs for this argument.
reviewboard/diffviewer/managers.py (Diff revision 12) |
---|
This says optional, but the argument doesn't have a default.
reviewboard/diffviewer/managers.py (Diff revision 12) |
---|
This is in the wrong spot with regards to the argument list.
Addressed Christian's feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 13 (+778 -224) |
Do not pass diffset_history to DiffCommitManager
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 14 (+775 -228) |
diff_commit_count
->diffcommit_count
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 15 (+775 -228) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 16 (+790 -230) |
reviewboard/diffviewer/filediff_creator.py (Diff revision 16) |
---|
The
(
should be on the next line. Kind of looks weird after another paren though.
Addressed Christian's feedback
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 17 (+779 -230) |
Fix
create_filediffs()
to save file counts.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 18 (+897 -232) |
reviewboard/diffviewer/tests/test_filediff_creator.py (Diff revision 18) |
---|
F401 'reviewboard.diffviewer.features.dvcs_feature' imported but unused
dev/release-4.0.x/dvcs/manager
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 19 (+896 -232) |