802932 - Refactor some pieces of try_parser.py

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Steve Fink [:sfink] [:s:]]

It is totally a judgement call whether the original or the new version is better, but I find the logic in the new version to be easier to follow. I don't think this changes any behavior, though it does strip out some redundant computations (which was really just a nesting level problem.)

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Refactor some pieces of try_parser.py

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Handle the 'none' value at the outer scope

Another refactoring. I prefer to do the parsing up-front in one place.

Comment 3

[[e-mail] (:bhearsum)]

Comment on attachment 672654 [details] [diff] [review]
Handle the 'none' value at the outer scope

Review of attachment 672654 [details] [diff] [review]:
-----------------------------------------------------------------

This patch looks good to me, feel free to land it on default anytime.

Comment 4

[[e-mail] (:bhearsum)]

Comment on attachment 672653 [details] [diff] [review]
Refactor some pieces of try_parser.py

Review of attachment 672653 [details] [diff] [review]:
-----------------------------------------------------------------

I think this all looks okay, modulo the one thing noted below. You can fix that upon landing without asking for another review if you want.

::: try_parser.py
@@ +53,5 @@
> +    if user_platforms == 'none':
> +        return []
> +
> +    # For tests, prettyNames contains list values (a list of slave builder
> +    # names?), and this should always return the empty list

Can you update this to explain why it should always be an empty list for tests?

Comment 5

[Steve Fink [:sfink] [:s:]]

(In reply to Ben Hearsum [:bhearsum] from comment #4)
> @@ +53,5 @@
> > +    if user_platforms == 'none':
> > +        return []
> > +
> > +    # For tests, prettyNames contains list values (a list of slave builder
> > +    # names?), and this should always return the empty list
> 
> Can you update this to explain why it should always be an empty list for
> tests?

Ok, I rewrote the comment. (Really, the control flow should be straightened out to avoid this case entirely, but for now this should do.)

New comment:

    # When prettyNames contains list values rather than simple strings, it
    # means that we're processing the argument for selecting test suites, so do
    # not return any build builders.

Comment 6

[[e-mail] (:bhearsum)]

(In reply to Steve Fink [:sfink] from comment #5)
> (In reply to Ben Hearsum [:bhearsum] from comment #4)
> > @@ +53,5 @@
> > > +    if user_platforms == 'none':
> > > +        return []
> > > +
> > > +    # For tests, prettyNames contains list values (a list of slave builder
> > > +    # names?), and this should always return the empty list
> > 
> > Can you update this to explain why it should always be an empty list for
> > tests?
> 
> Ok, I rewrote the comment. (Really, the control flow should be straightened
> out to avoid this case entirely, but for now this should do.)
> 
> New comment:
> 
>     # When prettyNames contains list values rather than simple strings, it
>     # means that we're processing the argument for selecting test suites, so
> do
>     # not return any build builders.

lgtm

Comment 7

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/build/buildbotcustom/rev/365bdcf9264b
http://hg.mozilla.org/build/buildbotcustom/rev/4d3ebfb6a4f5

Comment 8

[Kim Moir [:kmoir] ET]

In production

Comment 9

[Kim Moir [:kmoir] ET]

In production

Comment 10

[[e-mail] (:bhearsum)]

Steve - is there still one more patch you wanted to do here?