Fish Trophy

brennie got a fish trophy!

Fish Trophy

Add resources for interacting with DiffCommits

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

Information

Review Board
release-4.0.x
3118383...

Reviewers

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.

Description From Last Updated

E501 line too long (81 > 79 characters)

reviewbotreviewbot

F401 'django.utils.six' imported but unused

reviewbotreviewbot

F821 undefined name 'diff_commit'

reviewbotreviewbot

F821 undefined name 'diff_commit'

reviewbotreviewbot

F821 undefined name 'diff_commit'

reviewbotreviewbot

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

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

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

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

F811 redefinition of unused 'test_get_links_dvcs_enabled' from line 268

reviewbotreviewbot

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

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

F401 'django.utils.six' imported but unused

reviewbotreviewbot

F821 undefined name 'diff_commit'

reviewbotreviewbot

F821 undefined name 'diff_commit'

reviewbotreviewbot

F821 undefined name 'diff_commit'

reviewbotreviewbot

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

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

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

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

F811 redefinition of unused 'test_get_links_dvcs_enabled' from line 268

reviewbotreviewbot

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

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

F401 'reviewboard.diffviewer.models.DiffSet' imported but unused

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

This isn't in the function signature.

chipx86chipx86

Missing a docstring.

chipx86chipx86

Too many blank lines.

chipx86chipx86

The and .. or form was used in the past due to lack of the inline conditional support in earlier …

chipx86chipx86

"ID"

chipx86chipx86

We shouldn't use literals in summaries. Same below.

chipx86chipx86

You can just use ObjectDoesNotExist here.

chipx86chipx86

rsp

chipx86chipx86

"solely"

chipx86chipx86

Everything is indented 1 space in the body.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Trailing comma is turning this into a tuple. Same with other descriptions.

chipx86chipx86

Are we guaranteed that this is going to be compatible all the time with the args we put in? I'm …

chipx86chipx86

Can you make an explicit logger?

chipx86chipx86

Same comments as above.

chipx86chipx86

Incomplete docstring.

chipx86chipx86

Needs 2 blank lines between sections.

chipx86chipx86

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

chipx86chipx86

Too many blank lines.

chipx86chipx86

These can be combined.

chipx86chipx86

These too.

chipx86chipx86

Need tests for requesting patch content.

chipx86chipx86

Swap these.

chipx86chipx86

Blank line between these.

chipx86chipx86

E501 line too long (83 > 79 characters)

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

F401 'reviewboard.diffviewer.models.DiffSet' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

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

chipx86chipx86

Can you say "without commit history support" ?

chipx86chipx86

Can you change this to ReviewRequestDraft.publish?

chipx86chipx86

I think this can be reverted. The file isn't otherwise modified, and the blank line is there to keep the …

chipx86chipx86

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

chipx86chipx86

Swap these so they're in alphabetical order.

chipx86chipx86

Typo: "sytem" -> "system"

chipx86chipx86

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

chipx86chipx86

Should be DictFieldType.

chipx86chipx86

The type path must be updated for the new submodule. Same with any others in this change.

chipx86chipx86

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

chipx86chipx86

Same here.

chipx86chipx86

Missing period.

chipx86chipx86

You can combine these lines and wrap at a .

chipx86chipx86

Missing period.

chipx86chipx86

Typo: "Aditional" -> "Additional"

chipx86chipx86

Can you pass these as keyword arguments?

chipx86chipx86

Typo: "curren" -> "current".

chipx86chipx86

These should be in alphabetical order.

chipx86chipx86

No newline.

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

No trailing comma.

chipx86chipx86

Typo: "curren" -> "current"

chipx86chipx86

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

chipx86chipx86

Missing @webapi_test_template.

chipx86chipx86

Missing @webapi_test_template.

chipx86chipx86

Alphabetical order.

chipx86chipx86

Alphabetical order.

chipx86chipx86

This should be the full type path.

chipx86chipx86

django, not djblets.

chipx86chipx86

This was here before, but can you swap these?

chipx86chipx86

Typo: "of" -> "or"

chipx86chipx86

Can we leave these to the default serialization so that these date fields are represented the same as all other …

chipx86chipx86

Typo: "specifcally" -> "specifically"

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Missing trailing comma.

chipx86chipx86

Still missing @webapi_test_template.

chipx86chipx86

Still missing @webapi_test_template

chipx86chipx86

F401 'dateutil.parser.isoparse' imported but unused

reviewbotreviewbot

F401 'reviewboard.diffviewer.models.DiffCommit' imported but unused

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

flake8

brennie
Review request changed
Change Summary:

Flake8

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
Review request changed
Change Summary:

Nest draft diff commit resource under draft diff.
Allow creation of empty diffs.

Commit:
ddfec95aff77a82810d82ead65f2320f341cb952
cca60c9f7b49cf020b701c87922ab46220b48d63

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
Review request changed
Change Summary:

Fix some unit test failures. Serialize Diffset.diff_commit_count.

Commit:
b7d26e370530aa30d3311e6a4f8946a05c9fd1fd
80dae83a097b9718f49f3c7bfb1eb0d347fefdda

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

    This isn't in the function signature.

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

    Missing a docstring.

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

    Too many blank lines.

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

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

    "ID"

  7. Show all issues

    We shouldn't use literals in summaries.

    Same below.

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

    You can just use ObjectDoesNotExist here.

  9. Show all issues

    rsp

  10. Show all issues

    "solely"

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

    Everything is indented 1 space in the body.

  12. reviewboard/webapi/resources/diff_commit.py (Diff revision 10)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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?

  13. Show all issues

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

  14. reviewboard/webapi/resources/diff_commit.py (Diff revision 10)
     
     
     
     
    Show all issues

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

  15. Show all issues

    Trailing comma is turning this into a tuple.

    Same with other descriptions.

  16. Show all issues

    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.

  17. Show all issues

    Can you make an explicit logger?

  18. reviewboard/webapi/resources/draft_diff_commit.py (Diff revision 10)
     
     
     
     
     
    Show all issues

    Same comments as above.

  19. reviewboard/webapi/resources/review_request_draft.py (Diff revision 10)
     
     
     
     
     
     
    Show all issues

    Incomplete docstring.

  20. reviewboard/webapi/tests/mimetypes.py (Diff revision 10)
     
     
     
     
     
     
     
     
     
    Show all issues

    Needs 2 blank lines between sections.

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

    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.

  22. reviewboard/webapi/tests/test_diff.py (Diff revision 10)
     
     
     
     
     
     
     
    Show all issues

    Too many blank lines.

  23. reviewboard/webapi/tests/test_diff.py (Diff revision 10)
     
     
     
    Show all issues

    These can be combined.

  24. reviewboard/webapi/tests/test_diff.py (Diff revision 10)
     
     
     
    Show all issues

    These too.

  25. reviewboard/webapi/tests/test_diff_commit.py (Diff revision 10)
     
     
     
     
     
    Show all issues

    Need tests for requesting patch content.

  26. reviewboard/webapi/tests/test_draft_diff.py (Diff revision 10)
     
     
     
    Show all issues

    Swap these.

  27. Show all issues

    Blank line between these.

  28. 
      
brennie
Review request changed
Change Summary:

Addressed issues and rebased.

Commit:
469c80901ce5b33252e62b06a88da0ceccc4e8bd
2b33344865ae2441d22baf9e70be12f2c75da568

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
brennie
Review request changed
Change Summary:

Add unit tests for the last revision.

Commit:
76cddc14b207db1c81ec5ba6de8061d9d2ac658a
dd3d6d75c3489c09429e005d7eb09837a05a0e05

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

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

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

    Can you say "without commit history support" ?

  4. Show all issues

    Can you change this to ReviewRequestDraft.publish?

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

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

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

    Swap these so they're in alphabetical order.

  8. Show all issues

    Typo: "sytem" -> "system"

  9. Show all issues

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

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

    Should be DictFieldType.

  11. Show all issues

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

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

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

    Same here.

  14. Show all issues

    Missing period.

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

    You can combine these lines and wrap at a .

  16. Show all issues

    Missing period.

  17. Show all issues

    Typo: "Aditional" -> "Additional"

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

    Can you pass these as keyword arguments?

  19. Show all issues

    Typo: "curren" -> "current".

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

    These should be in alphabetical order.

  21. Show all issues

    No newline.

  22. Show all issues

    Missing a trailing comma.

  23. Show all issues

    No trailing comma.

  24. Show all issues

    Typo: "curren" -> "current"

  25. Show all issues

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

  26. Show all issues

    Missing @webapi_test_template.

  27. Show all issues

    Missing @webapi_test_template.

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

    Alphabetical order.

  29. reviewboard/webapi/tests/test_draft_diffcommit.py (Diff revision 16)
     
     
     
     
    Show all issues

    Alphabetical order.

  30. 
      
brennie
chipx86
  1. 
      
  2. reviewboard/reviews/forms.py (Diff revision 17)
     
     
    Show all issues

    This should be the full type path.

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

    django, not djblets.

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

    This was here before, but can you swap these?

  5. Show all issues

    Typo: "of" -> "or"

  6. Show all issues

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

    Typo: "specifcally" -> "specifically"

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

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

  9. Show all issues

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

  10. Show all issues

    Missing trailing comma.

  11. Show all issues

    Still missing @webapi_test_template.

  12. Show all issues

    Still missing @webapi_test_template

  13. 
      
brennie
Review request changed
Change Summary:

address feedback

Commit:
c0b700800a0acd74cf7f54beea88b8035e08b226
d79ea8ea8a89b776461fb1e85c7ad62246739580

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:
Completed
Change Summary:
Pushed to release-4.0.x (f03566e)