Code review comment for lp://qastaging/~liuyq0307/linaro-android-build-tools/add-monkey-l-a-t

On Mon, 26 Sep 2011 08:07:25 -0000
Yongqin Liu <email address hidden> wrote:

> > + image_type = os.environ.get("IMAGE_TYPE")
> >
> > What's being done here? If it calls for adding another build config
> > variable, it's likely superfluous (unless there're reasons why it's
> > not).
> sorry, I didn't recognize that this file only be used for
> android-build. I will delete the relevant source.

Thanks.

>
> > Also, braces placement needs fixing, it for sure worse than was
> > before. Rule of thumb is to just follow style of the existing code
> > (and minimize any whitespace changes).
> I have modify the braces placement according the policy that minimize
> any whitespace changes. BTW, is there any coding style for braces?

Well, we generally follow PEP8, though some grounded deviation may be
used. By general rule is the same as for C: everyone places braces in
different way, but when you submit patch for some code, you're expected
to follow the style of surrounding code. If you really think that
formatting should be changed, it should be separate commit - just
change formatting, no semantic changes.

> > And can you please look thru associated blueprint's whiteboard, it
> > seems to have typos which make it a bit unclear ("Acceptance:
> > Monkey for as long as it can", "Update the meeting to a weekly")
> I have updated the bp, is it ok now.
>
> And is there any environment that I can test this file?
> I want to see after the execution of this file, is the result
> displayed ok.

Thanks for bringing that up, that's how we'd like to handle such
changes for sure - test them on a sandbox before deploying on
production. I've created one at
https://ec2-75-101-195-88.compute-1.amazonaws.com/

Reviewing changes once again:

97 - "server": "http://validation.linaro.org/lava-server/dashboard",
98 + "server": "http://validation.linaro.org/lava-server/RPC2/",

Are you sure this is right? Why exactly such change is needed?

--
Best Regards,
Paul

Linaro.org | Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro
http://twitter.com/#!/linaroorg - http://www.linaro.org/linaro-blog