Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1918333005:
Add script to find unused Polymer elements (Closed)
|
Created:
4 years, 7 months ago by michaelpg Modified:
4 years, 7 months ago CC:
chromium-reviews, dbeam+watch-polymer_chromium.org, michaelpg+watch-polymer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd script to find unused Polymer elements
Detected three elements (removed in crrev.com/1930603003)
as well as the whitelisted elements.
Committed: https://crrev.com/d32a44f0b42dd5705d6dff278ee41b19c528829c
Cr-Commit-Position: refs/heads/master@{#391282}
BUG=605823
Patch Set 1 #Patch Set 2 : pylint #
Total comments: 20
Patch Set 3 : stevenjb #Patch Set 4 : simplify #Patch Set 5 : with #
Total comments: 4
Patch Set 6 : dbeam #Patch Set 7 : self #Messages
Total messages: 14 (5 generated)
michaelpg@chromium.org changed reviewers: + dbeam@chromium.org
dbeam, PTAL. I'm not used to our Python style guide but this passes pylint.
The style guide says to use 2-line indents, but neither Chromium nor Google says
anything about additional indents for continuation lines, so I interpret that to
mean this:
def MyFunc():
if SuperLongFunctionNameOne() and \
SuperLongFunctionNameTwo():
pass
is consistent with the style guide even though it is hard to read.
stevenjb@chromium.org changed reviewers: + stevenjb@chromium.org
Thanks, this seems much more maintainable to me, even if my python style fu is weak - it's better than my complete lack of bash fu, style or otherwise. https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... File third_party/polymer/v1_0/find_unused_elements.py (right): https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:18: import pyparsing Used? https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:21: import sys Used? https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:138: '/%s' % element, My python formatting fu is weak, but I suspect this should be on the previous line with the other arguments aligned. https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:146: break This would be more readable if the entire for loop were wrapped as a function with 'return True' instead of 'used = True'.
PTAL. I also ran flake8 which is kinda like closure + pylint. Re the indentation question, Chromium's guide says to fall back to PEP-8 for undecided issues, and PEP-8 intentionally leaves the issue open, so I'm erring on the side of clarity. https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... File third_party/polymer/v1_0/find_unused_elements.py (right): https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:18: import pyparsing On 2016/04/29 20:34:56, stevenjb wrote: > Used? Done. https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:21: import sys On 2016/04/29 20:34:55, stevenjb wrote: > Used? Done. https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:138: '/%s' % element, On 2016/04/29 20:34:56, stevenjb wrote: > My python formatting fu is weak, but I suspect this should be on the previous > line with the other arguments aligned. Done (both are valid, but your way looks better) https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:146: break On 2016/04/29 20:34:55, stevenjb wrote: > This would be more readable if the entire for loop were wrapped as a function > with 'return True' instead of 'used = True'. Meant to do that, forgot, done now. Thanks!
lgtm
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1918333005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1918333005/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add script to find unused Polymer elements Detected three elements (removed in crrev.com/1930603003) as well as the whitelisted elements. ========== to ========== Add script to find unused Polymer elements Detected three elements (removed in crrev.com/1930603003) as well as the whitelisted elements. Committed: https://crrev.com/d32a44f0b42dd5705d6dff278ee41b19c528829c Cr-Commit-Position: refs/heads/master@{#391282} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d32a44f0b42dd5705d6dff278ee41b19c528829c Cr-Commit-Position: refs/heads/master@{#391282}
Message was sent while issue was closed.
post-submit drive-by also, i think you're overusing @staticmethod here https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... File third_party/polymer/v1_0/find_unused_elements.py (right): https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:24: class UnusedElementsDetector: this should probably inherit from (object) https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:28: __whitelist = ( nit: __WHITELIST (Class contant https://google.github.io/styleguide/pyguide.html#Naming) https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:28: __whitelist = ( nit: I think it's more common to use 4\s indent in tuple literals https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:50: return re.sub('<!--.*?-->', '', text, flags=re.MULTILINE|re.DOTALL) nit: flags=re.MULTILINE | re.DOTALL) ^ ^ https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:65: text = re.sub('<if .*?>', '', text) nit: case-insensitive maybe? https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:156: UnusedElementsDetector.Run() if __name__ == '__main__': UnusedElementDetector().run() https://codereview.chromium.org/1918333005/diff/80001/third_party/polymer/v1_... File third_party/polymer/v1_0/find_unused_elements.py (right): https://codereview.chromium.org/1918333005/diff/80001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:110: continue what does this do? https://codereview.chromium.org/1918333005/diff/80001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:118: if len(unused_elements): i think this can be just if unused_elements:
Message was sent while issue was closed.
Description was changed from ========== Add script to find unused Polymer elements Detected three elements (removed in crrev.com/1930603003) as well as the whitelisted elements. Committed: https://crrev.com/d32a44f0b42dd5705d6dff278ee41b19c528829c Cr-Commit-Position: refs/heads/master@{#391282} ========== to ========== Add script to find unused Polymer elements Detected three elements (removed in crrev.com/1930603003) as well as the whitelisted elements. Committed: https://crrev.com/d32a44f0b42dd5705d6dff278ee41b19c528829c Cr-Commit-Position: refs/heads/master@{#391282} BUG=605823 ==========
Message was sent while issue was closed.
I addressed your comments and made some functions instance methods. Exact same patch is uploaded at https://codereview.chromium.org/1944133002 for CQ. https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... File third_party/polymer/v1_0/find_unused_elements.py (right): https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:24: class UnusedElementsDetector: On 2016/05/03 19:22:08, Dan Beam wrote: > this should probably inherit from (object) Done. https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:28: __whitelist = ( On 2016/05/03 19:22:08, Dan Beam wrote: > nit: __WHITELIST (Class contant > https://google.github.io/styleguide/pyguide.html#Naming) Done. https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:28: __whitelist = ( On 2016/05/03 19:22:08, Dan Beam wrote: > nit: I think it's more common to use 4\s indent in tuple literals the examples in PEP-8 use 4 spaces (as with other indents), the style guide says to use 2 spaces instead of 4 spaces, so that sounds "incorrect" https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:50: return re.sub('<!--.*?-->', '', text, flags=re.MULTILINE|re.DOTALL) On 2016/05/03 19:22:08, Dan Beam wrote: > nit: flags=re.MULTILINE | re.DOTALL) > ^ ^ Done. Looked weird to me inside a param=value expression with no spaces. https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:65: text = re.sub('<if .*?>', '', text) On 2016/05/03 19:22:07, Dan Beam wrote: > nit: case-insensitive maybe? Done. https://codereview.chromium.org/1918333005/diff/20001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:156: UnusedElementsDetector.Run() On 2016/05/03 19:22:07, Dan Beam wrote: > if __name__ == '__main__': > UnusedElementDetector().run() Done. https://codereview.chromium.org/1918333005/diff/80001/third_party/polymer/v1_... File third_party/polymer/v1_0/find_unused_elements.py (right): https://codereview.chromium.org/1918333005/diff/80001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:110: continue On 2016/05/03 19:22:08, Dan Beam wrote: > what does this do? removed https://codereview.chromium.org/1918333005/diff/80001/third_party/polymer/v1_... third_party/polymer/v1_0/find_unused_elements.py:118: if len(unused_elements): On 2016/05/03 19:22:08, Dan Beam wrote: > i think this can be just > > if unused_elements: Done. |
