Display diffstat information in the Commits table

RESOLVED FIXED

Status

MozReview
Review Board: Extension
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: smacleod, Assigned: davidwalsh)

Tracking

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
We should use the diffstat fetching functions from Bug 1286017 and display the information in the commits table. We could either do this in JS using the same payload the reviewer status stuff uses, or simply update the commits table template to output it when rendering.
(Assignee)

Comment 1

2 years ago
Created attachment 8774426 [details]
MozReview: Provide diffstat information in commits table (Bug 1286030)

Review commit: https://reviewboard.mozilla.org/r/66886/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66886/
(Assignee)

Comment 2

2 years ago
Comment on attachment 8774426 [details]
MozReview: Provide diffstat information in commits table (Bug 1286030)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66886/diff/1-2/
(Assignee)

Updated

2 years ago
Attachment #8774426 - Flags: review?(smacleod)
(Assignee)

Comment 3

2 years ago
Comment on attachment 8774426 [details]
MozReview: Provide diffstat information in commits table (Bug 1286030)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66886/diff/2-3/
(Assignee)

Comment 4

2 years ago
https://reviewboard.mozilla.org/r/66886/#review63730

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:52
(Diff revision 2)
>          <a title="See diff for commit {{child_details|commit_id|slice:":12"}}"
>             class="commit_sha" href="{{child_details.get_review_request.get_absolute_url}}diff/#index_header">
>            {{child_details|commit_id|slice:":12"}}
>          </a>
>        </td>
> +      <td class="diffstat truncate_text">

Maybe add `title=(output from fn)` so hover shows the full text in case it's truncated?

::: pylib/mozreview/mozreview/templatetags/mozreview.py:172
(Diff revision 2)
> +    if stat['insert'] != 0 and stat['delete'] != 0:
> +      separator = ' / '
> +    if stat['delete'] != 0:
> +      delete = '<span class="diffstat-delete">-%s</span>' % stat['delete']
> +
> +    return SafeString("%s%s%s" % (insert, separator, delete))

Using `SafeString` instead of `|safe` in the template
(Assignee)

Comment 5

2 years ago
Comment on attachment 8774426 [details]
MozReview: Provide diffstat information in commits table (Bug 1286030)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66886/diff/3-4/
(Assignee)

Comment 6

2 years ago
Comment on attachment 8774426 [details]
MozReview: Provide diffstat information in commits table (Bug 1286030)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66886/diff/4-5/
(Assignee)

Comment 7

2 years ago
https://reviewboard.mozilla.org/r/66886/#review63798

::: pylib/mozreview/mozreview/templatetags/mozreview.py:166
(Diff revision 5)
> +def diffstat_text(review_request, user):
> +    stat = get_diffstats(review_request, user)
> +    insert = separator = delete = ''
> +
> +    if stat['insert'] != 0:
> +      insert = '<span class="diffstat-insert">+%s</span>' % diffstat_rounded_label(stat['insert'])

Needs pep8 but every indentation I try bombs.

::: pylib/mozreview/mozreview/templatetags/mozreview.py:170
(Diff revision 5)
> +    if stat['insert'] != 0:
> +      insert = '<span class="diffstat-insert">+%s</span>' % diffstat_rounded_label(stat['insert'])
> +    if stat['insert'] != 0 and stat['delete'] != 0:
> +      separator = ' / '
> +    if stat['delete'] != 0:
> +      delete = '<span class="diffstat-delete">+%s</span>' % diffstat_rounded_label(stat['delete'])

Needs pep8 but every indentation I try bombs.
(Reporter)

Comment 8

2 years ago
https://reviewboard.mozilla.org/r/66886/#review63798

> Needs pep8 but every indentation I try bombs.

```Python
      insert = '<span class="diffstat-insert">+%s</span>' % (
          diffstat_rounded_label(stat['insert']))
```

> Needs pep8 but every indentation I try bombs.

```Python
      delete = '<span class="diffstat-delete">+%s</span>' % (
          diffstat_rounded_label(stat['delete']))
```
(Assignee)

Comment 9

2 years ago
Comment on attachment 8774426 [details]
MozReview: Provide diffstat information in commits table (Bug 1286030)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66886/diff/5-6/
(Reporter)

Comment 10

2 years ago
Comment on attachment 8774426 [details]
MozReview: Provide diffstat information in commits table (Bug 1286030)

https://reviewboard.mozilla.org/r/66886/#review64282

::: pylib/mozreview/mozreview/templatetags/mozreview.py:165
(Diff revision 6)
> +    if stat['insert'] != 0:
> +        insert = '<span class="diffstat-insert">+%s</span>' % (
> +            diffstat_rounded_label(stat['insert']))
> +    if stat['insert'] != 0 and stat['delete'] != 0:
> +        separator = ' / '
> +    if stat['delete'] != 0:
> +        delete = '<span class="diffstat-delete">+%s</span>' % (
> +            diffstat_rounded_label(stat['delete']))

please put blank lines between conditionals which are not tied together (There is a gotcha here where someone may think the second and third if statements are elif, etc).

Alternatively, I wonder if we should just be passing the insert and delete count into the template context and moving this markup / logic there? That would probably be preferable...

Feel free to take either approach.

::: pylib/mozreview/mozreview/templatetags/mozreview.py:171
(Diff revision 6)
> +        insert = '<span class="diffstat-insert">+%s</span>' % (
> +            diffstat_rounded_label(stat['insert']))
> +    if stat['insert'] != 0 and stat['delete'] != 0:
> +        separator = ' / '
> +    if stat['delete'] != 0:
> +        delete = '<span class="diffstat-delete">+%s</span>' % (

`-` sign instead of `+`
Attachment #8774426 - Flags: review?(smacleod) → review+
(Assignee)

Comment 13

2 years ago
Comment on attachment 8774426 [details]
MozReview: Provide diffstat information in commits table (Bug 1286030)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66886/diff/6-7/
Comment on attachment 8774426 [details]
MozReview: Provide diffstat information in commits table (Bug 1286030)

https://reviewboard.mozilla.org/r/66886/#review64656

This is awesome!
Attachment #8774426 - Flags: review+

Comment 15

2 years ago
Pushed by smacleod@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/c36264ec46f1
MozReview: Provide diffstat information in commits table r=gps,smacleod
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.