Add a manager for the DiffCommit model

Review Request #9631 — Created Feb. 12, 2018 and submitted

Information

Review Board
release-4.0.x
bc4b0ce...

Reviewers

The main logic of the DiffSetManager has been refactored into two
parts:

  • reviewboard.diffviewer.filediff_creator, which handles the
    actual processing of diff files into FileDiffs; and
  • DiffManagerBase, which provides a base implementation of
    create_from_upload for both the DiffSetManager and the
    DiffCommitManager.

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

reviewbotreviewbot

typo: reivewboard

daviddavid

Missing a period.

daviddavid

) should be on the line before.

daviddavid

Docstring?

daviddavid

There are so many args here, let's put one per line.

daviddavid

Can we put one arg per line?

daviddavid

) should be on the previous line and no trailing comma after revision=1

daviddavid

F841 local variable 'history' is assigned to but never used

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

While here, can you make request default to None and be optional? It's used for an eventual call to get_file_exists(), …

chipx86chipx86

"will be empty"

chipx86chipx86

Can you pass these as keyword arguments? It'll be easier for maintenance.

chipx86chipx86

"filenames"

chipx86chipx86

"filename"

chipx86chipx86

"filename"

chipx86chipx86

"filenames"

chipx86chipx86

Can you fix the indentation while you're here?

chipx86chipx86

Missing a docstring.

chipx86chipx86

Missing a docstring.

chipx86chipx86

This read better the way it was before.

chipx86chipx86

It's not valid to put more text here between sections.

chipx86chipx86

Can you revert this? It doesn't simplify the code any, and makes it less future-proof. Better to have one task …

chipx86chipx86

Like above, can we make this optional?

chipx86chipx86

Is this necessary, given that this is a new manager for a new model? If it is, we should document …

chipx86chipx86

Can you pass a keyword argument here, so it's self-describing?

chipx86chipx86

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 …

chipx86chipx86

Can you use a keyword argument here?

chipx86chipx86

Keyword arguments would also be great here.

chipx86chipx86

This is the manager for DiffSet. I'm not sure DiffCommit is right here?

chipx86chipx86

F841 local variable 'filediffs' is assigned to but never used

reviewbotreviewbot

Other such functions say "Subclasses." Would be nice to be consistent.

chipx86chipx86

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Swap these.

chipx86chipx86

Should be "BaseDiffManager", to match other base classes.

chipx86chipx86

Missing an entry for **kwargs.

chipx86chipx86

I think instead of copying all the keyword arguments from create_from_data, we just document **kwargs and say these are passed …

chipx86chipx86

No u prefix. Also missing docs for this argument.

chipx86chipx86

Missing docs for this argument.

chipx86chipx86

Missing a period.

chipx86chipx86

This says optional, but the argument doesn't have a default.

chipx86chipx86

This is in the wrong spot with regards to the argument list.

chipx86chipx86

Why don't we pass basedir anymore?

chipx86chipx86

Missing docs for **kwargs.

chipx86chipx86

"F" before "P".

chipx86chipx86

, optional

chipx86chipx86

Missing backtick and period.

chipx86chipx86

The ( should be on the next line. Kind of looks weird after another paren though.

chipx86chipx86

Blank line between these.

chipx86chipx86

F401 'reviewboard.diffviewer.features.dvcs_feature' imported but unused

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

brennie
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
david
  1. 
      
  2. reviewboard/diffviewer/managers.py (Diff revision 2)
     
     

    typo: reivewboard

  3. reviewboard/diffviewer/managers.py (Diff revision 2)
     
     

    Missing a period.

  4. reviewboard/diffviewer/managers.py (Diff revision 2)
     
     
     

    ) should be on the line before.

  5. reviewboard/diffviewer/managers.py (Diff revision 2)
     
     

    Docstring?

  6. reviewboard/diffviewer/managers.py (Diff revision 2)
     
     
     
     
     
     
     
     
     

    There are so many args here, let's put one per line.

  7. reviewboard/diffviewer/managers.py (Diff revision 2)
     
     

    Can we put one arg per line?

  8. ) should be on the previous line and no trailing comma after revision=1

  9. 
      
brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     
     

    While here, can you make request default to None and be optional? It's used for an eventual call to get_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).

  3. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     

    "will be empty"

  4. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     
     
     
     
     
     
     
     

    Can you pass these as keyword arguments? It'll be easier for maintenance.

  5. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     

    "filenames"

  6. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     

    "filename"

  7. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     

    "filename"

  8. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     

    "filenames"

  9. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     
     

    Can you fix the indentation while you're here?

  10. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     

    Missing a docstring.

  11. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     

    Missing a docstring.

  12. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     
     
     
     
     
     

    This read better the way it was before.

    1. The way it was before is no longer true.

    2. I meant the version without bullet points. The bullet points reads weird.

  13. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     
     
     
     

    It's not valid to put more text here between sections.

  14. 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.

  15. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     
     

    Like above, can we make this optional?

  16. 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.

    1. Accidental copy-paste refactor.

  17. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     

    Can you pass a keyword argument here, so it's self-describing?

  18. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     

    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 a DiffCommit. It'd be nice to consolidate as much of this functionality as possible. I imagine the base class could have its own implementation that DiffCommitManager could override, calling the parent with a diff_commit= argument.

    1. The latter also doesn't create a DiffSet.

      I've extracted more behaviour into DiffProcessor.create_filediffs (which at this point should be a function since its a giant code smell) but the arguments and models are just different enough to make further refactoring ugly.

  19. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     

    Can you use a keyword argument here?

  20. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     
     
     
     
     
     
     

    Keyword arguments would also be great here.

  21. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     
     
     

    This is the manager for DiffSet. I'm not sure DiffCommit is right here?

    1. This is describing why the function exists in the first place.

  22. 
      
brennie
Review request changed

Change Summary:

Addressed Christian's issues.

Commit:

-42dd02bbb4fb76a92d541e2484dea7a3629eb311
+fe865810a504254c78b57518da41cc185a53d915

Diff:

Revision 6 (+757 -201)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
chipx86
  1. 
      
  2. 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.

    1. Oops, meant to open an issue here. Think I bumped the keyboard shortcut to toggle.

  3. reviewboard/diffviewer/managers.py (Diff revision 8)
     
     

    Other such functions say "Subclasses." Would be nice to be consistent.

  4. 
      
brennie
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/managers.py (Diff revision 12)
     
     
     

    Swap these.

  3. reviewboard/diffviewer/managers.py (Diff revision 12)
     
     

    Should be "BaseDiffManager", to match other base classes.

  4. reviewboard/diffviewer/managers.py (Diff revision 12)
     
     

    Missing an entry for **kwargs.

  5. 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 to create_from_data. Makes it easier to maintain and prevents discrepencies.

  6. reviewboard/diffviewer/managers.py (Diff revision 12)
     
     

    No u prefix.

    Also missing docs for this argument.

  7. reviewboard/diffviewer/managers.py (Diff revision 12)
     
     

    Missing docs for this argument.

  8. reviewboard/diffviewer/managers.py (Diff revision 12)
     
     

    Missing a period.

  9. reviewboard/diffviewer/managers.py (Diff revision 12)
     
     
     

    This says optional, but the argument doesn't have a default.

  10. reviewboard/diffviewer/managers.py (Diff revision 12)
     
     
     

    This is in the wrong spot with regards to the argument list.

  11. reviewboard/diffviewer/managers.py (Diff revision 12)
     
     

    Missing docs for **kwargs.

  12. 
      
brennie
brennie
brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/managers.py (Diff revisions 12 - 15)
     
     

    Why don't we pass basedir anymore?

    1. The basedir is associated with the diffset and must be the repo root for a multi-commit RR. The diffcommit model doesnt store the basedir.

  3. 
      
brennie
chipx86
  1. 
      
  2. "F" before "P".

  3. , optional

  4. Missing backtick and period.

  5. reviewboard/diffviewer/filediff_creator.py (Diff revision 16)
     
     
     

    The ( should be on the next line. Kind of looks weird after another paren though.

  6. reviewboard/diffviewer/filediff_creator.py (Diff revision 16)
     
     
     

    Blank line between these.

  7. 
      
brennie
brennie
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (f8f584a)
Loading...