Fish Trophy

brennie got a fish trophy!

Add resources for interacting with DiffCommits

Review Request #9889 — Created May 2, 2018 and submitted

brennie
Review Board
release-4.0.x
9883
9905, 10118
3118383...
reviewboard

The DraftDiffCommitResource allows new commits to be uploaded to a
review request draft with history support and the DiffCommitResource
allows fetching information about created commits.

Ran unit tests.

  • 0
  • 0
  • 102
  • 1
  • 103
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

brennie
Review request changed

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
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
brennie
brennie
chipx86
  1. 
      
  2. reviewboard/reviews/forms.py (Diff revision 10)
     
     
     
     

    This isn't in the function signature.

  3. reviewboard/reviews/forms.py (Diff revision 10)
     
     

    Missing a docstring.

  4. reviewboard/reviews/forms.py (Diff revision 10)
     
     
     
     
     

    Too many blank lines.

  5. reviewboard/webapi/resources/__init__.py (Diff revision 10)
     
     
     

    The and .. or form was used in the past due to lack of the inline conditional support in earlier versions of Python. We should just use the inline form here.

  6. We shouldn't use literals in summaries.

    Same below.

  7. reviewboard/webapi/resources/diff_commit.py (Diff revision 10)
     
     
     

    You can just use ObjectDoesNotExist here.

  8. reviewboard/webapi/resources/diff_commit.py (Diff revision 10)
     
     
     
     
     
     
     
     
     
     

    Everything is indented 1 space in the body.

  9. reviewboard/webapi/resources/diff_commit.py (Diff revision 10)
     
     
     
     
     
     
     
     
     
     

    Why can't we just tack on ?commit-id=... onto the URL? Are we dealing with dynamic content in the link that could interfere with that?

  10. Let's explicitly compare obj against None, since the former's gotten us into trouble before with valid but falsy-evaluating objects.

  11. reviewboard/webapi/resources/diff_commit.py (Diff revision 10)
     
     
     
     

    Can you pass these params as keyword arguments, to help with readability?

  12. Trailing comma is turning this into a tuple.

    Same with other descriptions.

  13. Are we guaranteed that this is going to be compatible all the time with the args we put in?

    I'm tempted to say let's not provide the URL, but rather mention the resource we care about (maybe referencing the link name on the appropriate resource). Callers should not be hard-coding URLs anyway.

  14. Can you make an explicit logger?

  15. reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 10)
     
     
     
     
     

    Same comments as above.

  16. reviewboard/webapi/resources/review_request_draft.py (Diff revision 10)
     
     
     
     
     
     

    Incomplete docstring.

  17. reviewboard/webapi/tests/mimetypes.py (Diff revision 10)
     
     
     
     
     
     
     
     
     

    Needs 2 blank lines between sections.

  18. reviewboard/webapi/tests/test_diff.py (Diff revision 10)
     
     

    Can you use a keyword argument for True? I have no idea what this is from reading this code.

    This applies to all occurrences in the change.

  19. reviewboard/webapi/tests/test_diff.py (Diff revision 10)
     
     
     
     
     
     
     

    Too many blank lines.

  20. reviewboard/webapi/tests/test_diff.py (Diff revision 10)
     
     
     

    These can be combined.

  21. reviewboard/webapi/tests/test_diff.py (Diff revision 10)
     
     
     

    These too.

  22. reviewboard/webapi/tests/test_diff_commit.py (Diff revision 10)
     
     
     
     
     

    Need tests for requesting patch content.

  23. reviewboard/webapi/tests/test_draft_diff.py (Diff revision 10)
     
     
     

    Swap these.

  24. Blank line between these.

  25. 
      
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/parser.py (Diff revision 16)
     
     
     

    While here, can you change this to "Return" instead of "Returns?"

  3. reviewboard/reviews/forms.py (Diff revision 16)
     
     

    Can you say "without commit history support" ?

  4. Can you change this to ReviewRequestDraft.publish?

  5. reviewboard/testing/testcase.py (Diff revision 16)
     
     
     
     

    I think this can be reverted. The file isn't otherwise modified, and the blank line is there to keep the comment tied specifically to the setting of the ID.

  6. reviewboard/webapi/resources/diff.py (Diff revision 16)
     
     
     

    No need for the else:. That will be implied given the return in the if. It'll be nice to flatten this a bit.

  7. reviewboard/webapi/resources/diffcommit.py (Diff revision 16)
     
     
     

    Swap these so they're in alphabetical order.

  8. Typo: "sytem" -> "system"

  9. You don't need to set this. It will be 'commits', based on name.

  10. reviewboard/webapi/resources/diffcommit.py (Diff revision 16)
     
     
     

    Should be DictFieldType.

  11. The type path must be updated for the new submodule.

    Same with any others in this change.

  12. reviewboard/webapi/resources/diffcommit.py (Diff revision 16)
     
     
     
     

    Should be consistent between here and the previous function with regard to the blank line.

  13. reviewboard/webapi/resources/diffcommit.py (Diff revision 16)
     
     
     
     

    Same here.

  14. Missing period.

  15. reviewboard/webapi/resources/diffcommit.py (Diff revision 16)
     
     
     

    You can combine these lines and wrap at a .

  16. Missing period.

  17. Typo: "Aditional" -> "Additional"

  18. reviewboard/webapi/resources/diffcommit.py (Diff revision 16)
     
     
     
     
     
     
     

    Can you pass these as keyword arguments?

  19. Typo: "curren" -> "current".

  20. reviewboard/webapi/resources/draft_diffcommit.py (Diff revision 16)
     
     
     
     

    These should be in alphabetical order.

  21. Missing a trailing comma.

  22. No trailing comma.

  23. Typo: "curren" -> "current"

  24. Sentence-o: "filter our the" -> "filter the"

  25. Missing @webapi_test_template.

  26. Missing @webapi_test_template.

  27. reviewboard/webapi/tests/test_draft_diffcommit.py (Diff revision 16)
     
     
     
     

    Alphabetical order.

  28. reviewboard/webapi/tests/test_draft_diffcommit.py (Diff revision 16)
     
     
     
     

    Alphabetical order.

  29. 
      
brennie
chipx86
  1. 
      
  2. reviewboard/reviews/forms.py (Diff revision 17)
     
     

    This should be the full type path.

  3. reviewboard/reviews/forms.py (Diff revision 17)
     
     

    django, not djblets.

  4. reviewboard/webapi/resources/__init__.py (Diff revision 17)
     
     
     

    This was here before, but can you swap these?

  5. Typo: "of" -> "or"

  6. Can we leave these to the default serialization so that these date fields are represented the same as all other date fields in the API?

  7. Typo: "specifcally" -> "specifically"

  8. reviewboard/webapi/resources/draft_diffcommit.py (Diff revision 17)
     
     
     
     
     
     
     
     
     
     
     
     
     

    This shouldn't be needed, since you define model_parent_key above and the default implementation just fetches that from the object.

  9. Maybe add "in order to post to this resource" at the end of the sentence.

  10. Missing trailing comma.

  11. Still missing @webapi_test_template.

  12. Still missing @webapi_test_template

  13. 
      
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

chipx86
  1. I'm happy once Review Bot is.

  2. 
      
brennie
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

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