provide more than a single line of context in bugzilla comments

RESOLVED FIXED

Status

MozReview
Integration: Bugzilla
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: glob, Assigned: glob)

Tracking

Production

Details

MozReview Requests

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

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
when viewing the comment in bugzilla a single line of context isn't sufficient -
when a comment is created for a single line, send 5 lines of context to bugzilla.

if the reviewer selects multiple lines, only the selected lines need to be sent to bugzilla.
(Assignee)

Comment 1

2 years ago
Created attachment 8728795 [details]
MozReview Request: rbbz: fix flake8 issues (bug 1255278) r?mdoglio

Review commit: https://reviewboard.mozilla.org/r/39129/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39129/
Attachment #8728795 - Flags: review?(mdoglio)
(Assignee)

Comment 2

2 years ago
Created attachment 8728796 [details]
MozReview Request: rbbz: add 5 lines of context to single-line reviews (bug 1255278) r?mdoglio

When a review has selected a single line for context, add an additional 5 lines
of context to the comment created in Bugzilla.  This provides parity with
Splinter, and falls in line with most user's expectations.

Review commit: https://reviewboard.mozilla.org/r/39131/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39131/
Attachment #8728796 - Flags: review?(mdoglio)
Comment on attachment 8728795 [details]
MozReview Request: rbbz: fix flake8 issues (bug 1255278) r?mdoglio

https://reviewboard.mozilla.org/r/39129/#review35835

::: pylib/rbbz/rbbz/diffs.py:178
(Diff revision 1)
> -            if chars[index] == "&" and chars[index + 1] == "g": # ">".
> +            if chars[index] == "&" and chars[index + 1] == "g":  # ">".
>                  # Space character
> -                replace_chars.append(" ");
> +                replace_chars.append(" ")
>                  current_width += 1
>                  index += 4
>  
> -            elif chars[index] == "&" and chars[index + 1] == "m": # "—".
> +            elif chars[index] == "&" and chars[index + 1] == "m":  # "—".

Is the semicolon the only change in this block? I don't understand why rb shows all these yellow lines
Attachment #8728795 - Flags: review?(mdoglio) → review+
Attachment #8728796 - Flags: review?(mdoglio) → review+
Comment on attachment 8728796 [details]
MozReview Request: rbbz: add 5 lines of context to single-line reviews (bug 1255278) r?mdoglio

https://reviewboard.mozilla.org/r/39131/#review35839

::: pylib/rbbz/rbbz/diffs.py:253
(Diff revision 1)
> +    # If the reviewer selected a single line, comment with
> +    # 5 lines of context.
> +    if comment.num_lines == 1:
> +        if comment.first_line < 6:
> +            first_line = 1
> +            num_lines = comment.first_line
> +        else:
> +            first_line = comment.first_line - 5
> +            num_lines = 6
> +    else:
> +        first_line = comment.first_line
> +        num_lines = comment.num_lines

Should we include a few lines after the change?
(Assignee)

Comment 5

2 years ago
https://reviewboard.mozilla.org/r/39131/#review35839

> Should we include a few lines after the change?

i tried that however it then makes it unclear which actual line the review comment refers to.  as a point of reference splinter quotes the 5 prior lines.
(Assignee)

Comment 6

2 years ago
https://hg.mozilla.org/hgcustom/version-control-tools/rev/8b5f9f26971a
https://hg.mozilla.org/hgcustom/version-control-tools/rev/e7bd1a1bcd79
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Version: Development/Staging → Production
Comment on attachment 8728796 [details]
MozReview Request: rbbz: add 5 lines of context to single-line reviews (bug 1255278) r?mdoglio

https://reviewboard.mozilla.org/r/39131/#review35885

::: pylib/rbbz/rbbz/diffs.py:253
(Diff revision 1)
> +    # If the reviewer selected a single line, comment with
> +    # 5 lines of context.
> +    if comment.num_lines == 1:
> +        if comment.first_line < 6:
> +            first_line = 1
> +            num_lines = comment.first_line
> +        else:
> +            first_line = comment.first_line - 5
> +            num_lines = 6
> +    else:
> +        first_line = comment.first_line
> +        num_lines = comment.num_lines
> +

nit: this block should logically be under the comment below it
Attachment #8728796 - Flags: review+

Updated

2 years ago
Duplicate of this bug: 1162667
You need to log in before you can comment on or make changes to this bug.