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
Commit:
56c8f10ea2d2c8461f2cc3dd358062094b2652f7
0b5163d08571398f0e333492e06c5076e1c4bc09

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

    typo: reivewboard

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

    Missing a period.

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

    ) should be on the line before.

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

    Docstring?

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

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

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

    Can we put one arg per line?

  8. Show all issues

    ) 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)
     
     
     
    Show all issues

    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)
     
     
    Show all issues

    "will be empty"

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

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

  5. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     
    Show all issues

    "filenames"

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

    "filename"

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

    "filename"

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

    "filenames"

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

    Can you fix the indentation while you're here?

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

    Missing a docstring.

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

    Missing a docstring.

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

    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)
     
     
     
     
     
    Show all issues

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

  14. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     
     
    Show all issues

    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)
     
     
     
    Show all issues

    Like above, can we make this optional?

  16. reviewboard/diffviewer/managers.py (Diff revision 5)
     
     
     
     
     
     
     
    Show all issues

    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)
     
     
    Show all issues

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

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

    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)
     
     
    Show all issues

    Can you use a keyword argument here?

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

    Keyword arguments would also be great here.

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

    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

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)
     
     
    Show all issues

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

  4. 
      
brennie
brennie
Review request changed
Change Summary:

Rebase

Commit:
d4105b27bae9a276d23113124d95f09bae9a238a
0a91cb4e9b0c3ccf0478e4396aa0e72be96464ac

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

    Swap these.

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

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

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

    Missing an entry for **kwargs.

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

    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)
     
     
    Show all issues

    No u prefix.

    Also missing docs for this argument.

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

    Missing docs for this argument.

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

    Missing a period.

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

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

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

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

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

    Missing docs for **kwargs.

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

    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. Show all issues

    "F" before "P".

  3. Show all issues

    , optional

  4. Show all issues

    Missing backtick and period.

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

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

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

    Blank line between these.

  7. 
      
brennie
brennie
brennie
Review request changed
Change Summary:

Fix create_filediffs() to save file counts.

Commit:
0d4bf895516e1e6861d346f979299436f6d498a7
2e9a2fda87df8e434419b0fd9dd4b197d0f1a0e1

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. Ship It!
  2. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to release-4.0.x (f8f584a)