Code review comment for lp:~steve-mcintyre/linaro-chromiumos/python-rewrite

Revision history for this message
Loïc Minier (lool) wrote :

I submitted a branch based on yours for you to merge, as I felt it was simpler to show the changes in commits rather than explain them, then ask you to do them, then review them etc. These were all very simple changes, and overall your code was really good, thanks! See https://code.launchpad.net/~lool/linaro-chromiumos/python-rewrite/+merge/54551 for the mp.

There is one major issue with the new Python code: usage of os.system() is unchecked, which means we don't fail the run when any command fails. This really must be fixed; with the new run() wrapper I introduced, it should be simpler to add this in a single place.

Other things I had in mind:
- we should have a place to share code between the scripts
- we should move them out of remote and kill my linaro-chromiumos-build frontend
- (low priority) getopt doesn't integrate too nicely with Python in my experience, there are nicer ways to handle flags, but it works with getopt so we can keep it for now
- create_logdir() feels a bit specific; I think we should have an ensure_dir() thin wrapper around os.isdir() + os.makedirs(), and we should setup logdir early; perhaps it's easier to just expose it as an option; this would also allow merging logged_run() with run()
- I think we should use stderr consistently for our own output, but I'm open to discuss this
- the run() interface should take an array of commands rather than a command string; this is nicer to deal with when preparing commands before running them

« Back to merge proposal