Closed Bug 1124764 Opened 9 years ago Closed 9 years ago

Change dumpreview output to YAML

Categories

(MozReview Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gps, Assigned: gps)

Details

Attachments

(2 files, 1 obsolete file)

See description.
Attached file MozReview Request: bz://1124764/gps (obsolete) —
/r/2849 - testing: convert output of rbmanage dumpreview to YAML (bug 1124764)

Pull down this commit:

hg pull review -r 90d44913aec9304189d129007c3d50efb6e8789e
Comment on attachment 8553212 [details]
MozReview Request: bz://1124764/gps

/r/2849 - testing: convert output of rbmanage dumpreview to YAML (bug 1124764)
/r/2855 - testing: dump review info from review requests

Pull down these commits:

hg pull review -r 4227e666c95c9b51077d12cb3ae11b9e0564d488
Comment on attachment 8553212 [details]
MozReview Request: bz://1124764/gps

/r/2849 - testing: convert output of rbmanage dumpreview to YAML (bug 1124764)
/r/2855 - testing: dump review info from review requests

Pull down these commits:

hg pull review -r 4227e666c95c9b51077d12cb3ae11b9e0564d488
Attachment #8553212 - Flags: review?(smacleod)
Comment on attachment 8553212 [details]
MozReview Request: bz://1124764/gps

/r/2849 - testing: convert output of rbmanage dumpreview to YAML (bug 1124764)
/r/2855 - testing: dump review info from review requests

Pull down these commits:

hg pull review -r 4e54a6ac80e650a703fdcdf39a9fc3444bf08691
https://reviewboard.mozilla.org/r/2849/#review2139

I gave the first few tests a really throrough inspection to make sure things looked correct and then mostly just skimmed the rest. This looks good though, I like how the output is much less ambiguous.

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 2)
>  import yaml
>  
> -# TODO Use YAML.
> +# Teach yaml how to format OrderedDict.

nit: toss another blank here.

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 2)
>  import os
>  import sys
>  
> +from collections import OrderedDict

shouldn't we have these all grouped together?

::: hgext/reviewboard/tests/test-commits-deleted-no-obsolescence.t
(Diff revision 2)
> -      p2rb.discard_on_publish_rids: ["6"]
> +    p2rb.commits: '[["c5b850e249510046906bcb24f774635c4521a4a9", "2"], ["905ad211ecc6f024e1f0ffdbe084dd06cf28ae1c",
> +      "3"], ["68fdf92dbf149ab8afb8295a76b79fb82a9629b1", "4"], ["53b32d356f20f6730c14ec62c3706eba7e68e078",
> +      "5"], ["f466ed1de51670e583e11deb2f1022a342b52ccd", "6"]]'

Ya, if you know to expect this type of wrapping it's okay. It looks to be a sane predictable algorithm for the wrapping... I hope it doesn't cause problems with strange long lines.
https://reviewboard.mozilla.org/r/2855/#review2141

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 2)
> +    for review in rr.get_reviews(review_request_id=rr.id):

This will break if there are over 25 reviews, since it will paginate the response (probably will never happen, but still).

You can pass `max_results=200` here to increase the default but 200 is your limit. That'd probably be more than sufficient for tests (well, 25 probably is in all honesty).

The other option is to also print out `review.total_results`, which will be how many exist even if it is greater than the page size, and then truncate displaying at the first 25 etc.

Feel free to just make the decision to drop this issue, just wanted you to at least think about it.

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 2)
> +        for comment in review.get_diff_comments():

Same thing with respect to the pagination and result limit here. It's actually a lot more likely diffcomments could overflow.

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 2)
> +            #interfilediff = comment.get_interfilediff()

I'm torn on whether to include this. Without we can't test comments on interdiffs, but we probably never will anyways.
Attachment #8553212 - Flags: review?(smacleod) → review+
Comment on attachment 8553212 [details]
MozReview Request: bz://1124764/gps

https://reviewboard.mozilla.org/r/2847/#review2143

Ship It!
Comment on attachment 8553212 [details]
MozReview Request: bz://1124764/gps

/r/2849 - testing: convert output of rbmanage dumpreview to YAML (bug 1124764); r=smacleod
/r/2855 - testing: dump review info from review requests

Pull down these commits:

hg pull review -r 6f0f109f12c713a7236985e033481e02ddf1e0e9
Attachment #8553212 - Flags: review+ → review?(smacleod)
https://reviewboard.mozilla.org/r/2849/#review2145

Always look on the bright side of life.

I analyzed your Python changes and found 4 errors.

The following files were examined:

  testing/vcttesting/reviewboard/mach_commands.py

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 3)
> -    print('  Public: %s' % r.public)
> -    if r.bugs_closed:
> +yaml.add_representer(OrderedDict, ordered_dict_presenter,
> +        Dumper=yaml.SafeDumper)

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 3)
> -    print('  Commit ID: %s' % r.commit_id)
> +def _serialize_text(s):

E302: expected 2 blank lines, found 1
Separate top-level function and class definitions with two blank lines.

Method definitions inside a class are separated by a single blank line.

Extra blank lines may be used (sparingly) to separate groups of related
functions.  Blank lines may be omitted between a bunch of related
one-liners (e.g. a set of dummy implementations).

Use blank lines in functions, sparingly, to indicate logical sections.

Okay: def a():\n    pass\n\n\ndef b():\n    pass
Okay: def a():\n    pass\n\n\n# Foo\n# Bar\n\ndef b():\n    pass

E301: class Foo:\n    b = 0\n    def bar():\n        pass
E302: def a():\n    pass\n\ndef b(n):\n    pass
E303: def a():\n    pass\n\n\n\ndef b(n):\n    pass
E303: def a():\n\n\n\n    pass
E304: @decorator\n\ndef a():\n    pass


::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 3)
> -    for k, v in sorted(r.extra_data.iteritems()):
> +def serialize_review_requests(rr):

E302: expected 2 blank lines, found 1
Separate top-level function and class definitions with two blank lines.

Method definitions inside a class are separated by a single blank line.

Extra blank lines may be used (sparingly) to separate groups of related
functions.  Blank lines may be omitted between a bunch of related
one-liners (e.g. a set of dummy implementations).

Use blank lines in functions, sparingly, to indicate logical sections.

Okay: def a():\n    pass\n\n\ndef b():\n    pass
Okay: def a():\n    pass\n\n\n# Foo\n# Bar\n\ndef b():\n    pass

E301: class Foo:\n    b = 0\n    def bar():\n        pass
E302: def a():\n    pass\n\ndef b(n):\n    pass
E303: def a():\n    pass\n\n\n\ndef b(n):\n    pass
E303: def a():\n\n\n\n    pass
E304: @decorator\n\ndef a():\n    pass
https://reviewboard.mozilla.org/r/2855/#review2147

Always look on the bright side of life.

I analyzed your Python changes and found 1 errors.

The following files were examined:

  testing/vcttesting/reviewboard/mach_commands.py

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 3)
> +            #interfilediff = comment.get_interfilediff()

E265: block comment should start with '# '
Separate inline comments by at least two spaces.

An inline comment is a comment on the same line as a statement.  Inline
comments should be separated by at least two spaces from the statement.
They should start with a # and a single space.

Each line of a block comment starts with a # and a single space
(unless it is indented text inside the comment).

Okay: x = x + 1  # Increment x
Okay: x = x + 1    # Increment x
Okay: # Block comment
E261: x = x + 1 # Increment x
E262: x = x + 1  #Increment x
E262: x = x + 1  #  Increment x
E265: #Block comment
Comment on attachment 8553212 [details]
MozReview Request: bz://1124764/gps

/r/2849 - testing: convert output of rbmanage dumpreview to YAML (bug 1124764); r=smacleod
/r/2855 - testing: dump review info from review requests

Pull down these commits:

hg pull review -r 8775ff61a1ed1636aab7040bcd19cb8a3ad1e63f
https://reviewboard.mozilla.org/r/2849/#review2149

Always look on the bright side of life.

I analyzed your Python changes and found 1 errors.

The following files were examined:

  testing/vcttesting/reviewboard/mach_commands.py
https://reviewboard.mozilla.org/r/2855/#review2151

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/vcttesting/reviewboard/mach_commands.py
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #8553212 - Flags: review?(smacleod)
Attachment #8553212 - Attachment is obsolete: true
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: