Add a manager for the DiffCommit model

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

brennie
Review Board
release-4.0.x
9609
9642
4c79653...
reviewboard

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.

  • 1
  • 0
  • 40
  • 2
  • 43
Description From Last Updated
Missing docs for this argument. chipx86 chipx86
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
Review request changed

Change Summary:

Do not pass diffset_history to DiffCommitManager

Commit:

-cd5fe6e71269fd9fb4d55db20b56623e9675f99c
+4c79653909cc7459358efb3bb9314f5f615e59bf

Diff:

Revision 14 (+775 -228)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...