1286017 - Include diffstat information in the ReviewRequestSummary resource

Status: RESOLVED FIXED

Description

[Steven MacLeod [:smacleod]]

This will allow us to display the diffstat information in the BMO MozReview section.

Comment 1

[Steven MacLeod [:smacleod]]

Created attachment 8769846 [details]
mozreview: add functions for retrieving diffstat information (Bug 1286017).

This commit ignores extra information Review Board has, such as
information on how many lines were replaced, rather than just deleted
and inserted. This is done to keep things simple as the extra
information isn't always present and I haven't thought of a good way
to conditionally pass things along.

This also makes consuming this new data much simpler as two numbers
which will always be present are used. These numbers are also pretty
self explanatory.

In the future we might want to expand this, but we can always keep
the current numbers we provide and just add new information.

Review commit: https://reviewboard.mozilla.org/r/63536/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63536/

Comment 2

[Steven MacLeod [:smacleod]]

Created attachment 8769847 [details]
mozreview: cleanup identation in summary example (Bug 1286017).

Review commit: https://reviewboard.mozilla.org/r/63538/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63538/

Comment 3

[Steven MacLeod [:smacleod]]

Created attachment 8769848 [details]
mozreview: add diffstat information to the ReviewRequestSummaryResource (Bug 1286017).

Review commit: https://reviewboard.mozilla.org/r/63540/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63540/

Comment 4

[Steven MacLeod [:smacleod]]

Created attachment 8769849 [details]
mozreview: add diffstat information to test output (Bug 1286017).

Review commit: https://reviewboard.mozilla.org/r/63542/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63542/

Comment 5

[Byron Jones ‹:glob› 🎈]

Comment on attachment 8769846 [details]
mozreview: add functions for retrieving diffstat information (Bug 1286017).

https://reviewboard.mozilla.org/r/63536/#review60562

::: pylib/mozreview/mozreview/diffviewer/__init__.py:8
(Diff revision 1)
> +
> +def diffstats(diffset):
> +    """Return a dictionary of diffstat information for a diffset."""
> +    counts = diffset.get_total_line_counts()
> +
> +    # TODO: Take into account the non raw counts review board might

please file a bug for this TODO

::: pylib/mozreview/mozreview/diffviewer/__init__.py:28
(Diff revision 1)
> +    review_request = review_request.get_review_request()
> +
> +    if rev is None:
> +        # If the user is the submitter we might want to use the draft diffset.
> +        draft = review_request.get_draft(user=user)
> +        diffset = ((draft and draft.diffset) or

i'd like to see a testcase that ensures the counts are present and correct for submitter + draft changes.

Comment 6

[Byron Jones ‹:glob› 🎈]

Comment on attachment 8769847 [details]
mozreview: cleanup identation in summary example (Bug 1286017).

https://reviewboard.mozilla.org/r/63538/#review60564

Comment 7

[Byron Jones ‹:glob› 🎈]

Comment on attachment 8769848 [details]
mozreview: add diffstat information to the ReviewRequestSummaryResource (Bug 1286017).

https://reviewboard.mozilla.org/r/63540/#review60566

Comment 8

[Byron Jones ‹:glob› 🎈]

Comment on attachment 8769849 [details]
mozreview: add diffstat information to test output (Bug 1286017).

https://reviewboard.mozilla.org/r/63542/#review60568

Comment 9

[Steven MacLeod [:smacleod]]

Created attachment 8772125 [details]
mozreview: add testing for diffstats on drafts (Bug 1286017).

Review commit: https://reviewboard.mozilla.org/r/65024/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65024/

Comment 10

[Steven MacLeod [:smacleod]]

Comment on attachment 8769846 [details]
mozreview: add functions for retrieving diffstat information (Bug 1286017).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63536/diff/1-2/

Comment 11

[Steven MacLeod [:smacleod]]

Comment on attachment 8769847 [details]
mozreview: cleanup identation in summary example (Bug 1286017).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63538/diff/1-2/

Comment 12

[Steven MacLeod [:smacleod]]

Comment on attachment 8769848 [details]
mozreview: add diffstat information to the ReviewRequestSummaryResource (Bug 1286017).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63540/diff/1-2/

Comment 13

[Steven MacLeod [:smacleod]]

Comment on attachment 8769849 [details]
mozreview: add diffstat information to test output (Bug 1286017).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63542/diff/1-2/

Comment 14

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/63540/#review61994

::: pylib/mozreview/mozreview/resources/review_request_summary.py:213
(Diff revision 2)
>          {
>              'commit': 'ece2029d013af68f9f32aa0a6199fcb2201d5aae',
> +            'diff': {
> +                'insert': 15,
> +                'delete': 20
> +            }

Missing a comma here.

Comment 15

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/63540/#review61994

> Missing a comma here.

Nope, this is example json and following the convention of the rest of the payload. Dropping :D

Comment 16

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/63540/#review61994

> Nope, this is example json and following the convention of the rest of the payload. Dropping :D

woops I'm silly, will fix :D

Comment 17

[Steven MacLeod [:smacleod]]

Comment on attachment 8769848 [details]
mozreview: add diffstat information to the ReviewRequestSummaryResource (Bug 1286017).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63540/diff/2-3/

Comment 18

[Steven MacLeod [:smacleod]]

Comment on attachment 8769849 [details]
mozreview: add diffstat information to test output (Bug 1286017).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63542/diff/2-3/

Comment 19

[Steven MacLeod [:smacleod]]

Comment on attachment 8772125 [details]
mozreview: add testing for diffstats on drafts (Bug 1286017).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65024/diff/1-2/

Comment 20

[Mark Côté [:mcote]]

Comment on attachment 8772125 [details]
mozreview: add testing for diffstats on drafts (Bug 1286017).

https://reviewboard.mozilla.org/r/65024/#review62000

smacleod asked me to look at this.  lgtm

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/08364a5147e6
mozreview: add functions for retrieving diffstat information . r=glob
https://hg.mozilla.org/hgcustom/version-control-tools/rev/d4a6879dbf0b
mozreview: cleanup identation in summary example . r=glob
https://hg.mozilla.org/hgcustom/version-control-tools/rev/d3c0c1ca5572
mozreview: add diffstat information to the ReviewRequestSummaryResource . r=glob
https://hg.mozilla.org/hgcustom/version-control-tools/rev/4222094d9397
mozreview: add diffstat information to test output . r=glob
https://hg.mozilla.org/hgcustom/version-control-tools/rev/cd617ebf2dd7
mozreview: add testing for diffstats on drafts . r=mcote