Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
539603003:
Beginnings of flag validator to compare GN and gyp build flags (Closed)
DescriptionCommitted: https://crrev.com/e0c052dd70a7515239a5d9e32c1b768a3781af66
Cr-Commit-Position: refs/heads/master@{#293209}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : tidy #
Total comments: 12
Patch Set 7 : review #
Total comments: 1
Messages
Total messages: 16 (6 generated)
scottmg@chromium.org changed reviewers: + ajwong@chromium.org, brettw@chromium.org
I guess start with this, add it to a bot for a small target, and see how it goes.
Mostly style nits and some python hygiene. https://codereview.chromium.org/539603003/diff/100001/tools/gn/bin/gyp_flag_c... File tools/gn/bin/gyp_flag_compare.py (right): https://codereview.chromium.org/539603003/diff/100001/tools/gn/bin/gyp_flag_c... tools/gn/bin/gyp_flag_compare.py:1: #!/usr/bin/env python Add a newline? https://codereview.chromium.org/539603003/diff/100001/tools/gn/bin/gyp_flag_c... tools/gn/bin/gyp_flag_compare.py:16: that takes a value as a separate entry. Modifies |command_line| in place.""" Follow docstring style? https://google-styleguide.googlecode.com/svn/trunk/pyguide.html https://codereview.chromium.org/539603003/diff/100001/tools/gn/bin/gyp_flag_c... tools/gn/bin/gyp_flag_compare.py:29: command_line[i:i + 2] = [command_line[i] + ' ' + command_line[i + 1]] Why not just create a new array and return? This looks n^2 due to shuffling and it's really confusing when you mutate an array you're iterating over. https://codereview.chromium.org/539603003/diff/100001/tools/gn/bin/gyp_flag_c... tools/gn/bin/gyp_flag_compare.py:84: return 1 Use True and False? https://codereview.chromium.org/539603003/diff/100001/tools/gn/bin/gyp_flag_c... tools/gn/bin/gyp_flag_compare.py:92: gyp = open(sys.argv[1], 'rb').readlines() Nit..with syntax is a little cleaner. with open(...) as gyp: blah https://codereview.chromium.org/539603003/diff/100001/tools/gn/bin/gyp_flag_c... tools/gn/bin/gyp_flag_compare.py:98: diffs = 0 True and False?
Thanks https://codereview.chromium.org/539603003/diff/100001/tools/gn/bin/gyp_flag_c... File tools/gn/bin/gyp_flag_compare.py (right): https://codereview.chromium.org/539603003/diff/100001/tools/gn/bin/gyp_flag_c... tools/gn/bin/gyp_flag_compare.py:1: #!/usr/bin/env python On 2014/09/03 20:25:35, awong wrote: > Add a newline? Done. M-A always wants it removed, so now I'm all over the place. :) https://codereview.chromium.org/539603003/diff/100001/tools/gn/bin/gyp_flag_c... tools/gn/bin/gyp_flag_compare.py:16: that takes a value as a separate entry. Modifies |command_line| in place.""" On 2014/09/03 20:25:35, awong wrote: > Follow docstring style? > > https://google-styleguide.googlecode.com/svn/trunk/pyguide.html Done. https://codereview.chromium.org/539603003/diff/100001/tools/gn/bin/gyp_flag_c... tools/gn/bin/gyp_flag_compare.py:29: command_line[i:i + 2] = [command_line[i] + ' ' + command_line[i + 1]] On 2014/09/03 20:25:35, awong wrote: > Why not just create a new array and return? This looks n^2 due to shuffling and > it's really confusing when you mutate an array you're iterating over. Done. https://codereview.chromium.org/539603003/diff/100001/tools/gn/bin/gyp_flag_c... tools/gn/bin/gyp_flag_compare.py:84: return 1 On 2014/09/03 20:25:35, awong wrote: > Use True and False? Done. (was just because it's used as a main() return code) https://codereview.chromium.org/539603003/diff/100001/tools/gn/bin/gyp_flag_c... tools/gn/bin/gyp_flag_compare.py:92: gyp = open(sys.argv[1], 'rb').readlines() On 2014/09/03 20:25:35, awong wrote: > Nit..with syntax is a little cleaner. > > with open(...) as gyp: > blah Done. https://codereview.chromium.org/539603003/diff/100001/tools/gn/bin/gyp_flag_c... tools/gn/bin/gyp_flag_compare.py:98: diffs = 0 On 2014/09/03 20:25:35, awong wrote: > True and False? Done.
LGTM https://codereview.chromium.org/539603003/diff/120001/tools/gn/bin/gyp_flag_c... File tools/gn/bin/gyp_flag_compare.py (right): https://codereview.chromium.org/539603003/diff/120001/tools/gn/bin/gyp_flag_c... tools/gn/bin/gyp_flag_compare.py:16: that takes a value as a separate entry. If I was being really picky, I'd say this first sentence should be at max 1 line long, and that you need to document arguments. But this script doesn't look like it's going to last long enough to care so I will just passive aggressively leave this comment and then LG anyways. :)
On 2014/09/03 20:46:37, awong wrote: > LGTM > > https://codereview.chromium.org/539603003/diff/120001/tools/gn/bin/gyp_flag_c... > File tools/gn/bin/gyp_flag_compare.py (right): > > https://codereview.chromium.org/539603003/diff/120001/tools/gn/bin/gyp_flag_c... > tools/gn/bin/gyp_flag_compare.py:16: that takes a value as a separate entry. > If I was being really picky, I'd say this first sentence should be at max 1 line > long, and that you need to document arguments. But this script doesn't look like > it's going to last long enough to care so I will just passive aggressively leave > this comment and then LG anyways. :) Yeah, not sure if it's worth checking in, but seems a little better than just leaving it here (but I can just do that too). Brett, OK for step 1? I can also do some sort of clumping first to see if that makes it more useful/actionable.
rs lgtm
On 2014/09/03 21:09:17, scottmg wrote: > On 2014/09/03 20:46:37, awong wrote: > > LGTM > > > > > https://codereview.chromium.org/539603003/diff/120001/tools/gn/bin/gyp_flag_c... > > File tools/gn/bin/gyp_flag_compare.py (right): > > > > > https://codereview.chromium.org/539603003/diff/120001/tools/gn/bin/gyp_flag_c... > > tools/gn/bin/gyp_flag_compare.py:16: that takes a value as a separate entry. > > If I was being really picky, I'd say this first sentence should be at max 1 > line > > long, and that you need to document arguments. But this script doesn't look > like > > it's going to last long enough to care so I will just passive aggressively > leave > > this comment and then LG anyways. :) > > Yeah, not sure if it's worth checking in, but seems a little better than just > leaving it here (but I can just do that too). Check it in! > > Brett, OK for step 1? I can also do some sort of clumping first to see if that > makes it more useful/actionable.
The CQ bit was checked by scottmg@chromium.org
The CQ bit was unchecked by scottmg@chromium.org
The CQ bit was checked by scottmg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/539603003/120001
The CQ bit was unchecked by scottmg@chromium.org
The CQ bit was checked by scottmg@chromium.org
Message was sent while issue was closed.
Committed patchset #7 (id:120001) to pending queue manually as bdc642a (presubmit successful).
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e0c052dd70a7515239a5d9e32c1b768a3781af66 Cr-Commit-Position: refs/heads/master@{#293209} |
