Andreas Tolfsen ❲:ato❳ Reporter
	Description • 9 years ago

Issues raised on Review Board appear as comments on Bugzilla.  Unfortunately the comment doesn't contain a line number which means you have to look up the issue on RB to find where in the file the issue is.

Example comment (from https://bugzilla.mozilla.org/show_bug.cgi?id=1107706#c60):

    ::: testing/marionette/driver.js
    (Diff revision 7)
    > +  let listenerWindow = Services.wm.getOuterWindowWithId(id);

It would be good if the first line could be changed to include for example the first line of the selection, like this:

    ::: testing/arionette/driver.js:541

	Dan Minor [:dminor] Assignee
	Comment 1 • 9 years ago

/r/5863 - rbbz: include line numbers for issues in bugzilla comments (bug 1144653); r=smacleod

Pull down this commit:

hg pull review -r 780864f44682bca7a8d718927ebecfe0d94ef792

	Steven MacLeod [:smacleod]
	Comment 2 • 9 years ago

https://reviewboard.mozilla.org/r/5863/#review5689

::: pylib/rbbz/rbbz/diffs.py
(Diff revision 1)
> +    if left_lineno != right_lineno:
> +        lineno = '%s|%s' % (left_lineno, right_lineno)
> +    else:
> +        lineno = str(left_lineno)
> +
>      lines = [
> -        "::: %s" % comment.filediff.dest_file_display,
> +        "::: %s:%s" % (comment.filediff.dest_file_display, lineno),

I'm not sure if we should provide both line numbers here when we only ever display the destination (right side) file name. I think some might confuse the two line number format for a line + column number as well.

::: pylib/rbbz/rbbz/diffs.py
(Diff revision 1)
> +            if line[0] == comment.first_line:
> +                left_lineno = line[1]
> +                right_lineno = line[4]

I can't remember what the default value is here for a number that doesn't exist (e.g. it's an added block, or remove block, so only one side has a line). I'm assuming though it's not equal to the other line number, which means whenever we hit an add or remove block we'll switch to the two line number mode below - but one of them will be a bogus value such as an empty string or `None`.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 3 • 9 years ago

https://reviewboard.mozilla.org/r/5863/#review5691

> I can't remember what the default value is here for a number that doesn't exist (e.g. it's an added block, or remove block, so only one side has a line). I'm assuming though it's not equal to the other line number, which means whenever we hit an add or remove block we'll switch to the two line number mode below - but one of them will be a bogus value such as an empty string or `None`.

Just starting line number is enough information to go by.  The important thing here is to be able to see from a Bugzilla comment where to start looking, instead of having to then open the issue on Mozreview to get the context.

	Dan Minor [:dminor] Assignee
	Comment 4 • 9 years ago

https://reviewboard.mozilla.org/r/5863/#review5813

> Just starting line number is enough information to go by.  The important thing here is to be able to see from a Bugzilla comment where to start looking, instead of having to then open the issue on Mozreview to get the context.

It's the empty string, so we'd get e.g. 10: or :10, but since we're switching to displaying just the destination line number, it doesn't really matter.

	Dan Minor [:dminor] Assignee
	Comment 5 • 9 years ago

Comment on attachment 8581653 [details]
MozReview Request: bz://1144653/dminor

/r/5863 - rbbz: include line numbers for issues in bugzilla comments (bug 1144653); r=smacleod

Pull down this commit:

hg pull -r 72cc8521200f72aca0562e23b459ed0831dee04c https://reviewboard-hg.mozilla.org/version-control-tools/

	Steven MacLeod [:smacleod]
	Comment 6 • 9 years ago

https://reviewboard.mozilla.org/r/5863/#review5875

Ya this might end up being a little strange for comments starting in remove blocks, but I'm happy with this for now.

	Steven MacLeod [:smacleod]
	Comment 7 • 9 years ago

Comment on attachment 8581653 [details]
MozReview Request: bz://1144653/dminor

https://reviewboard.mozilla.org/r/5861/#review5877

Ship It!

	Dan Minor [:dminor] Assignee
	Comment 8 • 9 years ago

Thanks for the review! Pushed to: https://hg.mozilla.org/hgcustom/version-control-tools/rev/d2c1862d8c18