Closed Bug 1286030 Opened 8 years ago Closed 8 years ago

Display diffstat information in the Commits table

Categories

(MozReview Graveyard :: Review Board: Extension, defect)

Product:
MozReview Graveyard
Component:
Review Board: Extension
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: smacleod, Assigned: davidwalsh)

References

Details

Attachments

(1 file)

MozReview: Provide diffstat information in commits table (Bug 1286030)
58 bytes, text/x-review-board-request
smacleod
: review+
gps
: review+
Details 
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.
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/
Attachment #8774426 - Flags: review?(smacleod)
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/
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
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/
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/
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.
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']))
```
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/
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+
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+
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
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: