Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2345703004:
web_dev_style: ignore --css-vars from alphabetical ordering requirement (Closed)
Descriptionweb_dev_style: ignore --css-vars from alphabetical ordering requirement
R=dschuyler@chromium.org,stevenjb@chromium.org
BUG=647491
NOTRY=true
Committed: https://crrev.com/2cc1577903139bfb611a87973a6aca648ea2675d
Cr-Commit-Position: refs/heads/master@{#420434}
Patch Set 1 : such tragic misspelling #
Total comments: 9
Patch Set 2 : combine mixin+var logic #
Total comments: 5
Patch Set 3 : more review stuff #
Total comments: 4
Patch Set 4 : dschuyler review #
Messages
Total messages: 26 (14 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== web_dev_style: ignore --css-vars from alphabetical ordering requirement R=dschuyler@chromium.org,stevenjb@chromium.org BUG=none ========== to ========== web_dev_style: ignore --css-vars from alphabetical ordering requirement R=dschuyler@chromium.org,stevenjb@chromium.org BUG=647491 ==========
Thanks for doing this so quickly. https://codereview.chromium.org/2345703004/diff/20001/chrome/browser/web_dev_... File chrome/browser/web_dev_style/css_checker.py (right): https://codereview.chromium.org/2345703004/diff/20001/chrome/browser/web_dev_... chrome/browser/web_dev_style/css_checker.py:67: return re.sub(re.compile(r'--[\d\w-]+: [^;]+?;', re.DOTALL), '', s); How about entries like this --foo: { option: choice; }; I think it may leave the "};" https://codereview.chromium.org/2345703004/diff/20001/chrome/browser/web_dev_... chrome/browser/web_dev_style/css_checker.py:67: return re.sub(re.compile(r'--[\d\w-]+: [^;]+?;', re.DOTALL), '', s); I think the \d isn't needed (if I'm wrong on that it would help me to know). https://codereview.chromium.org/2345703004/diff/20001/chrome/browser/web_dev_... File chrome/browser/web_dev_style/css_checker_test.py (right): https://codereview.chromium.org/2345703004/diff/20001/chrome/browser/web_dev_... chrome/browser/web_dev_style/css_checker_test.py:140: --aardvark-animal: var(--zzyxz-xylophone); maybe add: --two-line: var(--second-line); --like-a-mixin: { foo: 4px; bar: 5px; };
https://codereview.chromium.org/2345703004/diff/20001/chrome/browser/web_dev_... File chrome/browser/web_dev_style/css_checker.py (right): https://codereview.chromium.org/2345703004/diff/20001/chrome/browser/web_dev_... chrome/browser/web_dev_style/css_checker.py:67: return re.sub(re.compile(r'--[\d\w-]+: [^;]+?;', re.DOTALL), '', s); On 2016/09/15 23:17:36, dschuyler wrote: > How about entries like this > --foo: { > option: choice; > }; > > I think it may leave the "};" does that mean you want me to combine this with _remove_mixins?
https://codereview.chromium.org/2345703004/diff/20001/chrome/browser/web_dev_... File chrome/browser/web_dev_style/css_checker.py (right): https://codereview.chromium.org/2345703004/diff/20001/chrome/browser/web_dev_... chrome/browser/web_dev_style/css_checker.py:67: return re.sub(re.compile(r'--[\d\w-]+: [^;]+?;', re.DOTALL), '', s); On 2016/09/15 23:30:17, Dan Beam wrote: > On 2016/09/15 23:17:36, dschuyler wrote: > > How about entries like this > > --foo: { > > option: choice; > > }; > > > > I think it may leave the "};" > > does that mean you want me to combine this with _remove_mixins? Ah, I see it now. Maybe just a comment saying that _remove_mixins() should be run prior this function. Though my preference would be to combine them because it makes the order clear an removes a function call. Either is good with me. Mostly my comment was me not noticing that the mixins would have already been removed. https://codereview.chromium.org/2345703004/diff/20001/chrome/browser/web_dev_... File chrome/browser/web_dev_style/css_checker_test.py (right): https://codereview.chromium.org/2345703004/diff/20001/chrome/browser/web_dev_... chrome/browser/web_dev_style/css_checker_test.py:140: --aardvark-animal: var(--zzyxz-xylophone); On 2016/09/15 23:17:36, dschuyler wrote: > maybe add: > > --two-line: > var(--second-line); > > --like-a-mixin: { > foo: 4px; > bar: 5px; > }; With my better understanding of the mixins being removed first, I see that the --like-a-mixin test is not a good addition.
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/2345703004/diff/20001/chrome/browser/web_dev_... File chrome/browser/web_dev_style/css_checker.py (right): https://codereview.chromium.org/2345703004/diff/20001/chrome/browser/web_dev_... chrome/browser/web_dev_style/css_checker.py:67: return re.sub(re.compile(r'--[\d\w-]+: [^;]+?;', re.DOTALL), '', s); On 2016/09/15 23:48:35, dschuyler wrote: > On 2016/09/15 23:30:17, Dan Beam wrote: > > On 2016/09/15 23:17:36, dschuyler wrote: > > > How about entries like this > > > --foo: { > > > option: choice; > > > }; > > > > > > I think it may leave the "};" > > > > does that mean you want me to combine this with _remove_mixins? > > Ah, I see it now. Maybe just a comment saying that > _remove_mixins() should be run prior this function. > Though my preference would be to combine them because > it makes the order clear an removes a function call. > Either is good with me. > > Mostly my comment was me not noticing that the mixins > would have already been removed. Done.
The CQ bit was checked by dbeam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2345703004/diff/20001/chrome/browser/web_dev_... File chrome/browser/web_dev_style/css_checker.py (right): https://codereview.chromium.org/2345703004/diff/20001/chrome/browser/web_dev_... chrome/browser/web_dev_style/css_checker.py:67: return re.sub(re.compile(r'--[\d\w-]+: [^;]+?;', re.DOTALL), '', s); On 2016/09/15 23:17:36, dschuyler wrote: > I think the \d isn't needed (if I'm wrong on > that it would help me to know). I'm pretty sure \d is redundant with \w within a [] block. https://codereview.chromium.org/2345703004/diff/60001/chrome/browser/web_dev_... File chrome/browser/web_dev_style/css_checker.py (left): https://codereview.chromium.org/2345703004/diff/60001/chrome/browser/web_dev_... chrome/browser/web_dev_style/css_checker.py:56: return re.sub(re.compile(r'--[\d\w-]+: {.*?};', re.DOTALL), '', s) With this no longer being done, it looks like something like this, "--foo: {\n a: b;\n c: d;\n};\n" would result in, '\n c: d;\n};\n' based on the new _remove_valid_vars(). https://codereview.chromium.org/2345703004/diff/60001/chrome/browser/web_dev_... File chrome/browser/web_dev_style/css_checker.py (right): https://codereview.chromium.org/2345703004/diff/60001/chrome/browser/web_dev_... chrome/browser/web_dev_style/css_checker.py:70: return re.sub(re.compile(valid_var_reg, re.DOTALL), '', s) About the regex changes: - having \d and \w in the same [] is be redundant (previously mentioned). - I think the re.DOTALL doesn't affect a regex that doesn't have any dots '.' - If the re.DOTALL is removed, the re.compile would become redundant. - the \s* near the end looks redundant with the [^;]+, since 'not ;' would include \s. - the ? near the end "[^;]+?;" (while harmless) doesn't appear to be useful - if the ? was trying to make the "[^;]+" optional, then either of these would work as alternatives: "([^;]+)?" or "[^;]*" - the backslashes happen to be working out ok with \w and \s, but I think it would be more correct to create raw (r'') strings Does something like the following do what is needed? mixin_shim_reg = r'[\w-]+_-_[\w-]+' def _remove_valid_vars(s): valid_var_reg = '--(?!' + mixin_shim_reg + r')[\w-]+\s*:[^;]*;' return re.sub(valid_var_reg, '', s) https://codereview.chromium.org/2345703004/diff/60001/chrome/browser/web_dev_... File chrome/browser/web_dev_style/css_checker_test.py (right): https://codereview.chromium.org/2345703004/diff/60001/chrome/browser/web_dev_... chrome/browser/web_dev_style/css_checker_test.py:165: color: red; Adding more elements to this test should show the error in not stripping the mixins. e.g. color: red; zebra: animal; cow: dog; Currently it would leave the "};" which doesn't generate an error.
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/2345703004/diff/20001/chrome/browser/web_dev_... File chrome/browser/web_dev_style/css_checker.py (right): https://codereview.chromium.org/2345703004/diff/20001/chrome/browser/web_dev_... chrome/browser/web_dev_style/css_checker.py:67: return re.sub(re.compile(r'--[\d\w-]+: [^;]+?;', re.DOTALL), '', s); On 2016/09/19 21:23:28, dschuyler wrote: > On 2016/09/15 23:17:36, dschuyler wrote: > > I think the \d isn't needed (if I'm wrong on > > that it would help me to know). > > I'm pretty sure \d is redundant with \w within > a [] block. Done. https://codereview.chromium.org/2345703004/diff/60001/chrome/browser/web_dev_... File chrome/browser/web_dev_style/css_checker.py (right): https://codereview.chromium.org/2345703004/diff/60001/chrome/browser/web_dev_... chrome/browser/web_dev_style/css_checker.py:70: return re.sub(re.compile(valid_var_reg, re.DOTALL), '', s) On 2016/09/19 21:23:28, dschuyler wrote: > About the regex changes: > - having \d and \w in the same [] is be redundant (previously mentioned). Done. > - I think the re.DOTALL doesn't affect a regex that doesn't have any dots '.' added back a . > - If the re.DOTALL is removed, the re.compile would become redundant. i kept it, but turns out you can just pass flags= as well. changed all _remove* methods to just use re.sub(..., flags=...) rather than compile() > - the \s* near the end looks redundant with the [^;]+, since 'not ;' would > include \s. Done. > - the ? near the end "[^;]+?;" (while harmless) doesn't appear to be useful Done. > - if the ? was trying to make the "[^;]+" optional, then either of these would > work as alternatives: "([^;]+)?" or "[^;]*" no, we talked about this. was supposed to be non-greedy. it probably doesn't matter. dropped other than for .*? > - the backslashes happen to be working out ok with \w and \s, but I think it > would be more correct to create raw (r'') strings Done. > > Does something like the following do what is needed? > > mixin_shim_reg = r'[\w-]+_-_[\w-]+' > > def _remove_valid_vars(s): > valid_var_reg = '--(?!' + mixin_shim_reg + r')[\w-]+\s*:[^;]*;' > return re.sub(valid_var_reg, '', s) kind of, but like you mentioned (somewhere else?) [^;] can only be satisfied until the first ;. inside of multi-rule mixins, this is problematic. so i've essentially created 2 branches: if we find a {.*?}, remove that, else look until ; https://codereview.chromium.org/2345703004/diff/60001/chrome/browser/web_dev_... File chrome/browser/web_dev_style/css_checker_test.py (right): https://codereview.chromium.org/2345703004/diff/60001/chrome/browser/web_dev_... chrome/browser/web_dev_style/css_checker_test.py:165: color: red; On 2016/09/19 21:23:28, dschuyler wrote: > Adding more elements to this test should > show the error in not stripping the mixins. > e.g. > color: red; > zebra: animal; > cow: dog; > > Currently it would leave the "};" which > doesn't generate an error. Done.
dbeam@chromium.org changed reviewers: - stevenjb@chromium.org
Description was changed from ========== web_dev_style: ignore --css-vars from alphabetical ordering requirement R=dschuyler@chromium.org,stevenjb@chromium.org BUG=647491 ========== to ========== web_dev_style: ignore --css-vars from alphabetical ordering requirement R=dschuyler@chromium.org,stevenjb@chromium.org BUG=647491 NOTRY=true ==========
lgtm https://codereview.chromium.org/2345703004/diff/100001/chrome/browser/web_dev... File chrome/browser/web_dev_style/css_checker.py (right): https://codereview.chromium.org/2345703004/diff/100001/chrome/browser/web_dev... chrome/browser/web_dev_style/css_checker.py:32: s = _remove_grit(s) # Must be done first. Thanks for this comment! https://codereview.chromium.org/2345703004/diff/100001/chrome/browser/web_dev... chrome/browser/web_dev_style/css_checker.py:65: return re.sub(not_shim + name + mixin_or_value, '', s, flags=re.DOTALL) optional: not_shim is actually '--' plus the not shim. I thought about suggesting that the '--' be moved to line 65, but then I also wondered if name would still be simple enough as name = r'--(?!' + mixin_shim_reg + r')[\w-]+:\s*' and skip the not_shim var. Or maybe a different name for not_shim which doesn't sound like a simple inverse (complement) of shim. Though it's not a big deal to leave it as is.
https://codereview.chromium.org/2345703004/diff/100001/chrome/browser/web_dev... File chrome/browser/web_dev_style/css_checker.py (right): https://codereview.chromium.org/2345703004/diff/100001/chrome/browser/web_dev... chrome/browser/web_dev_style/css_checker.py:32: s = _remove_grit(s) # Must be done first. On 2016/09/22 18:58:44, dschuyler wrote: > Thanks for this comment! Acknowledged. https://codereview.chromium.org/2345703004/diff/100001/chrome/browser/web_dev... chrome/browser/web_dev_style/css_checker.py:65: return re.sub(not_shim + name + mixin_or_value, '', s, flags=re.DOTALL) On 2016/09/22 18:58:44, dschuyler wrote: > optional: not_shim is actually '--' > plus the not shim. > > I thought about suggesting that > the '--' be moved to line 65, but then > I also wondered if name would still be > simple enough as > name = r'--(?!' + mixin_shim_reg + r')[\w-]+:\s*' > and skip the not_shim var. > > Or maybe a different name for not_shim > which doesn't sound like a simple inverse > (complement) of shim. > > Though it's not a big deal to leave it > as is. Done.
The CQ bit was checked by dbeam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/2345703004/#ps120001 (title: "dschuyler review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== web_dev_style: ignore --css-vars from alphabetical ordering requirement R=dschuyler@chromium.org,stevenjb@chromium.org BUG=647491 NOTRY=true ========== to ========== web_dev_style: ignore --css-vars from alphabetical ordering requirement R=dschuyler@chromium.org,stevenjb@chromium.org BUG=647491 NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== web_dev_style: ignore --css-vars from alphabetical ordering requirement R=dschuyler@chromium.org,stevenjb@chromium.org BUG=647491 NOTRY=true ========== to ========== web_dev_style: ignore --css-vars from alphabetical ordering requirement R=dschuyler@chromium.org,stevenjb@chromium.org BUG=647491 NOTRY=true Committed: https://crrev.com/2cc1577903139bfb611a87973a6aca648ea2675d Cr-Commit-Position: refs/heads/master@{#420434} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2cc1577903139bfb611a87973a6aca648ea2675d Cr-Commit-Position: refs/heads/master@{#420434} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
