Include diffstat information in the ReviewRequestSummary resource

RESOLVED FIXED

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: smacleod, Assigned: smacleod)

Tracking

(Blocks: 1 bug)

Details

Attachments

(5 attachments)

(Assignee)

Description

2 years ago
This will allow us to display the diffstat information in the BMO MozReview section.
(Assignee)

Updated

2 years ago
Blocks: 1286027
(Assignee)

Updated

2 years ago
Blocks: 1286030
(Assignee)

Comment 1

2 years ago
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/
Attachment #8769846 - Flags: review?(glob)
Attachment #8769847 - Flags: review?(glob)
Attachment #8769848 - Flags: review?(glob)
Attachment #8769849 - Flags: review?(glob)
(Assignee)

Comment 2

2 years ago
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/
(Assignee)

Comment 3

2 years ago
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/
(Assignee)

Comment 4

2 years ago
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 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.
Attachment #8769846 - Flags: review?(glob) → review+
Attachment #8769847 - Flags: review?(glob) → review+
Comment on attachment 8769847 [details]
mozreview: cleanup identation in summary example (Bug 1286017).

https://reviewboard.mozilla.org/r/63538/#review60564
Comment on attachment 8769848 [details]
mozreview: add diffstat information to the ReviewRequestSummaryResource (Bug 1286017).

https://reviewboard.mozilla.org/r/63540/#review60566
Attachment #8769848 - Flags: review?(glob) → review+
Comment on attachment 8769849 [details]
mozreview: add diffstat information to test output (Bug 1286017).

https://reviewboard.mozilla.org/r/63542/#review60568
Attachment #8769849 - Flags: review?(glob) → review+
(Assignee)

Updated

2 years ago
Blocks: 1287551
(Assignee)

Comment 9

2 years ago
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/
Attachment #8772125 - Flags: review?(glob)
(Assignee)

Comment 10

2 years ago
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/
(Assignee)

Comment 11

2 years ago
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/
(Assignee)

Comment 12

2 years ago
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/
(Assignee)

Comment 13

2 years ago
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/
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.
(Assignee)

Comment 15

2 years ago
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
(Assignee)

Comment 16

2 years ago
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
(Assignee)

Comment 17

2 years ago
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/
(Assignee)

Comment 18

2 years ago
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/
(Assignee)

Comment 19

2 years ago
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 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
Attachment #8772125 - Flags: review+

Updated

2 years ago
Attachment #8772125 - Flags: review?(glob)

Comment 21

2 years ago
Pushed by smacleod@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.