1255278 - provide more than a single line of context in bugzilla comments

Status: RESOLVED FIXED

Description

[Byron Jones ‹:glob› 🎈]

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.

Comment 1

[Byron Jones ‹:glob› 🎈]

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/

Comment 2

[Byron Jones ‹:glob› 🎈]

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/

Comment 3

[Mauro Doglio [: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

Comment 4

[Mauro Doglio [:mdoglio]]

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?

Comment 5

[Byron Jones ‹:glob› 🎈]

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.

Comment 6

[Byron Jones ‹:glob› 🎈]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/8b5f9f26971a
https://hg.mozilla.org/hgcustom/version-control-tools/rev/e7bd1a1bcd79

Comment 7

[Steven MacLeod [:smacleod]]

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