Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1553993002:
Add a Python parser for GN types to gn_helpers. (Closed)
|
Created:
4 years, 11 months ago by brettw Modified:
4 years, 11 months ago Reviewers:
Dirk Pranke CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a Python parser for GN types to gn_helpers.
Some scripts take serialized GN values including strings and lists. These are serialized and escaped according to GN rules. In particular, the string escaping is not possible to duplicate with Python's "ast" module. This simple parser will convert values properly.
Add unit tests for new code, as well as existing code in gn_helpers.
Fix omissions in existing ToGNString code.
BUG=573132, 571022
Committed: https://crrev.com/9dffb54bc426d84e4a2a1a6fb2989bf53237f5a2
Cr-Commit-Position: refs/heads/master@{#371000}
Patch Set 1 #Patch Set 2 : comments #
Total comments: 15
Patch Set 3 : #
Messages
Total messages: 23 (11 generated)
brettw@chromium.org changed reviewers: + dpranke@chromium.org
I realize the parsing code looks like C++ but it mirrors the structure of the GN parser which I consider an advantage in this case.
Description was changed from ========== Add a Python parser for GN types to gn_helpers. Some scripts take serialized GN values including strings and lists. These are serialized and escaped according to GN rules. In particular, the string escaping is not possible to duplicate with Python's "ast" module. This simple parser will convert values properly. Add unit tests for new code, as well as existing code in gn_helpers. Fix omissions in existing ToGNString code. ========== to ========== Add a Python parser for GN types to gn_helpers. Some scripts take serialized GN values including strings and lists. These are serialized and escaped according to GN rules. In particular, the string escaping is not possible to duplicate with Python's "ast" module. This simple parser will convert values properly. Add unit tests for new code, as well as existing code in gn_helpers. Fix omissions in existing ToGNString code. BUG=573132 ==========
The CQ bit was checked by brettw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1553993002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1553993002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by brettw@chromium.org
Description was changed from ========== Add a Python parser for GN types to gn_helpers. Some scripts take serialized GN values including strings and lists. These are serialized and escaped according to GN rules. In particular, the string escaping is not possible to duplicate with Python's "ast" module. This simple parser will convert values properly. Add unit tests for new code, as well as existing code in gn_helpers. Fix omissions in existing ToGNString code. BUG=573132 ========== to ========== Add a Python parser for GN types to gn_helpers. Some scripts take serialized GN values including strings and lists. These are serialized and escaped according to GN rules. In particular, the string escaping is not possible to duplicate with Python's "ast" module. This simple parser will convert values properly. Add unit tests for new code, as well as existing code in gn_helpers. Fix omissions in existing ToGNString code. BUG=573132,571022 ==========
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1553993002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1553993002/20001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
ping
sorry for the delay (so much backlog!) ... I've started looking at this but it'll take me a little while longer to finish; should be done this afternoon.
lgtm w/ various nits ... https://codereview.chromium.org/1553993002/diff/20001/build/gn_helpers.py File build/gn_helpers.py (right): https://codereview.chromium.org/1553993002/diff/20001/build/gn_helpers.py#new... build/gn_helpers.py:91: def UnescapeGNString(value): Is this supposed to be a public function, or should it be private to this file? https://codereview.chromium.org/1553993002/diff/20001/build/gn_helpers.py#new... build/gn_helpers.py:103: if next_char == '$' or next_char == '"' or next_char == '\\': nit: maybe if next_char in ('$', '"', '\\'): https://codereview.chromium.org/1553993002/diff/20001/build/gn_helpers.py#new... build/gn_helpers.py:108: # Any other backslash is a litera. typo: s/litera/literal/ . https://codereview.chromium.org/1553993002/diff/20001/build/gn_helpers.py#new... build/gn_helpers.py:116: class GNValueParser: nit: inherit from object, like: class GNValueParser(object): Also, is this object supposed to be public? https://codereview.chromium.org/1553993002/diff/20001/build/gn_helpers.py#new... build/gn_helpers.py:158: def ParseAllowTrailing(self): if this and the following functions are not public, they should be named _ParseAllowTrailing() et al, and some of your assertions are really kinda unneeded at that point. https://codereview.chromium.org/1553993002/diff/20001/build/gn_helpers.py#new... build/gn_helpers.py:167: elif next_char in "-0123456789": nit: consider making this a helper function: IsDigitOrMinus() or some such thing, and re-use here and on line 186? You might also rewrite this as next_char.isdigit() or next_char == '-', but I admit that I'm not sure if that's either faster or clearer. https://codereview.chromium.org/1553993002/diff/20001/build/gn_helpers.py#new... build/gn_helpers.py:188: while not self.IsDone() and self.input[self.cur] in '0123456789': maybe self.input[self.cur].isdigit() ? maybe not ... alternatively, why not match against a regex? to better mirror the c++ code, maybe? https://codereview.chromium.org/1553993002/diff/20001/build/gn_helpers.py#new... build/gn_helpers.py:226: raise GNException('Expected string but got nothing.') 'Expected list' instead? https://codereview.chromium.org/1553993002/diff/20001/build/gn_helpers.py#new... build/gn_helpers.py:261: def ConstantFollows(self, constant): nit: _ConstantFollows() if this isn't a public function.
https://codereview.chromium.org/1553993002/diff/20001/build/gn_helpers.py File build/gn_helpers.py (right): https://codereview.chromium.org/1553993002/diff/20001/build/gn_helpers.py#new... build/gn_helpers.py:91: def UnescapeGNString(value): On 2016/01/22 00:34:02, Dirk Pranke (slow) wrote: > Is this supposed to be a public function, or should it be private to this file? I was thinking public since one could conceive of needing such a helper function in the future. https://codereview.chromium.org/1553993002/diff/20001/build/gn_helpers.py#new... build/gn_helpers.py:103: if next_char == '$' or next_char == '"' or next_char == '\\': On 2016/01/22 00:34:02, Dirk Pranke (slow) wrote: > nit: maybe > > if next_char in ('$', '"', '\\'): Done. https://codereview.chromium.org/1553993002/diff/20001/build/gn_helpers.py#new... build/gn_helpers.py:116: class GNValueParser: On 2016/01/22 00:34:02, Dirk Pranke (slow) wrote: > nit: inherit from object, like: > > class GNValueParser(object): > > Also, is this object supposed to be public? Yes, if you want to parse a specific type or something. https://codereview.chromium.org/1553993002/diff/20001/build/gn_helpers.py#new... build/gn_helpers.py:188: while not self.IsDone() and self.input[self.cur] in '0123456789': On 2016/01/22 00:34:02, Dirk Pranke (slow) wrote: > maybe self.input[self.cur].isdigit() ? maybe not ... > > alternatively, why not match against a regex? to better mirror the c++ code, > maybe? I don't think in regex's and the code is longer. I did isdigit() https://codereview.chromium.org/1553993002/diff/20001/build/gn_helpers.py#new... build/gn_helpers.py:226: raise GNException('Expected string but got nothing.') On 2016/01/22 00:34:02, Dirk Pranke (slow) wrote: > 'Expected list' instead? Done. https://codereview.chromium.org/1553993002/diff/20001/build/gn_helpers.py#new... build/gn_helpers.py:261: def ConstantFollows(self, constant): On 2016/01/22 00:34:02, Dirk Pranke (slow) wrote: > nit: _ConstantFollows() if this isn't a public function. Done.
The CQ bit was checked by brettw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/1553993002/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1553993002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1553993002/40001
Message was sent while issue was closed.
Description was changed from ========== Add a Python parser for GN types to gn_helpers. Some scripts take serialized GN values including strings and lists. These are serialized and escaped according to GN rules. In particular, the string escaping is not possible to duplicate with Python's "ast" module. This simple parser will convert values properly. Add unit tests for new code, as well as existing code in gn_helpers. Fix omissions in existing ToGNString code. BUG=573132,571022 ========== to ========== Add a Python parser for GN types to gn_helpers. Some scripts take serialized GN values including strings and lists. These are serialized and escaped according to GN rules. In particular, the string escaping is not possible to duplicate with Python's "ast" module. This simple parser will convert values properly. Add unit tests for new code, as well as existing code in gn_helpers. Fix omissions in existing ToGNString code. BUG=573132,571022 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add a Python parser for GN types to gn_helpers. Some scripts take serialized GN values including strings and lists. These are serialized and escaped according to GN rules. In particular, the string escaping is not possible to duplicate with Python's "ast" module. This simple parser will convert values properly. Add unit tests for new code, as well as existing code in gn_helpers. Fix omissions in existing ToGNString code. BUG=573132,571022 ========== to ========== Add a Python parser for GN types to gn_helpers. Some scripts take serialized GN values including strings and lists. These are serialized and escaped according to GN rules. In particular, the string escaping is not possible to duplicate with Python's "ast" module. This simple parser will convert values properly. Add unit tests for new code, as well as existing code in gn_helpers. Fix omissions in existing ToGNString code. BUG=573132,571022 Committed: https://crrev.com/9dffb54bc426d84e4a2a1a6fb2989bf53237f5a2 Cr-Commit-Position: refs/heads/master@{#371000} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9dffb54bc426d84e4a2a1a6fb2989bf53237f5a2 Cr-Commit-Position: refs/heads/master@{#371000} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
