Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
476453002:
Python readability review for dbeam@. (Closed)
|
Created:
6 years, 4 months ago by Dan Beam Modified:
5 years, 9 months ago CC:
chromium-reviews, dbeam+watch-closure_chromium.org, Tyler Breisacher (Chromium), Jamie, garykac Base URL:
svn://svn.chromium.org/chrome/trunk/src Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPython readability review for dbeam@.
This simplifies the python closure compiler infrastructure.
R=rothwell@google.com
Committed: https://crrev.com/999677e0674bf4dae5e4a0b03e25947d6d8a844d
Cr-Commit-Position: refs/heads/master@{#322255}
Patch Set 1 #
Total comments: 2
Patch Set 2 : merge #
Total comments: 42
Patch Set 3 : rothwell@ review #
Total comments: 2
Patch Set 4 : remove space #
Messages
Total messages: 41 (19 generated)
https://codereview.chromium.org/476453002/diff/1/third_party/closure_compiler... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/476453002/diff/1/third_party/closure_compiler... third_party/closure_compiler/checker.py:67: def _clean_up(self): it seems like _protected_method() is preferred and __private_method() is discouraged in newer python style guides whereas __private_method() is still in the older ones. https://codereview.chromium.org/476453002/diff/1/third_party/closure_compiler... File third_party/closure_compiler/processor_test.py (right): https://codereview.chromium.org/476453002/diff/1/third_party/closure_compiler... third_party/closure_compiler/processor_test.py:44: def testInline(self): testName is 10x more popular than test_name in chrome code $ git grep -E '^\s+def test_.*\(' -- '*.py' | wc -l 253 $ git grep -E '^\s+def test[A-Z].*\(' -- '*.py' | wc -l 2246 despite the style guide's guidance of method_names().
dbeam@chromium.org changed reviewers: + krs@google.com
dbeam@chromium.org changed reviewers: + rothwell@google.com - krs@google.com
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Apologies for how long it took me to get to this https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:1: #!/usr/bin/python are there unittests for this file? https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:62: _found_java = False add a comment? https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:62: _found_java = False why is this a class variable and not an instance variable (comment) https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:64: def __init__(self, verbose=False): no docstring? https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:83: print "(INFO) %s" % msg there's no logging module that you could/should be using here? https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:85: def _error(self, msg): maybe this should be called _log_error - I would expect a method named like this to raise an exception https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:87: self._clean_up() this seems like a confusing side effect of this method https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:93: cmd: A list of tokens to be joined into a shell command. minor nit but indention here differs from google3 - chromium is different? https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:96: True if the exit code was 0, else False. this doesn't look correct - it appears to return the process handle. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:105: def _check_java_path(self): maybe this should be a classmethod as it modifies class state https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:123: """Changes a line number from /tmp/file:300 to /orig/file:100. maybe elaborate on why this conversion is necessary? why would the line numbers in the tmp file not match the original? https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:173: (exitcode, output) The exit code of the Closure compiler (as a number) is exitcode useful to the caller? I would expect a bool to make more sense. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:186: return this return code doesn't match your stated API https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:191: rel_path = lambda f: os.path.join(os.path.relpath(os.getcwd(), tmp_dir), f) not sure this lambda is buying you much, I would just move this into the list comprehension https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:231: return bool(errors), output your API says you will return an integer, this is a boolean https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:252: exit, _ = checker.check(source, depends=depends, externs=externs) why return an error string if it's just going to be ignored? https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:261: open(opts.out_file, "w").write("") it seems confusing to write an empty file - shouldn't this feature just be unimplemented for now, rather than incompletely implemented? https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... File third_party/closure_compiler/processor.py (right): https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/processor.py:24: class FileCache(object): It doesn't look like this needs to be a class, it can just be a function. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/processor.py:78: def __init__(self, source_file): docstring - e.g. what is a source_file, a filename or a file object https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/processor.py:83: while self._index < len(self._lines): I think you want to be using enumerate here, or document why you can't https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/processor.py:123: def included_files(self): if you're trying to obscure access to the underlying data structure this should return a copy of the list. otherwise you should just make included_files a public field
Patchset #5 (id:160001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:180001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:200001) has been deleted
On 2014/12/18 22:30:48, rothwell wrote: > Apologies for how long it took me to get to this Heh, it's all good; was just trying to get to this before being OOO for a while (part of why I've lagged returning your comments). re: logging/testing -- I think it'd be a lot easier for both of us if I just write followup CLs and we defer any readability granting until you're satisfied (rather than keep heaping on changes in this same CL). https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:1: #!/usr/bin/python On 2014/12/18 22:30:47, rothwell wrote: > are there unittests for this file? this file mainly just runs a .jar file. the majority of trickiness is in Processor, IMO. what would you have me test? the arguments passed to the jar? maybe the error formatting/manipulation? actually running the compiler itself seems more like a functional or integration test than a unit test (as it'd be testing more than this component). we do have those tests, and I'm working on getting them running continuously soon. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:62: _found_java = False On 2014/12/18 22:30:47, rothwell wrote: > why is this a class variable and not an instance variable (comment) I've just removed checking for java on the user's path. The script can just crash and burn in this case. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:64: def __init__(self, verbose=False): On 2014/12/18 22:30:47, rothwell wrote: > no docstring? Done. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:83: print "(INFO) %s" % msg On 2014/12/18 22:30:48, rothwell wrote: > there's no logging module that you could/should be using here? this CL is getting kinda big. can I do this separately for you? fwiw: not sure how much we use the logging module in chrome. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:85: def _error(self, msg): On 2014/12/18 22:30:47, rothwell wrote: > maybe this should be called _log_error - I would expect a method named like this > to raise an exception Done. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:87: self._clean_up() On 2014/12/18 22:30:47, rothwell wrote: > this seems like a confusing side effect of this method This originally had a reason to be here, but now I don't see why it should be. Removed. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:93: cmd: A list of tokens to be joined into a shell command. On 2014/12/18 22:30:47, rothwell wrote: > minor nit but indention here differs from google3 - chromium is different? Done. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:96: True if the exit code was 0, else False. On 2014/12/18 22:30:47, rothwell wrote: > this doesn't look correct - it appears to return the process handle. Done. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:105: def _check_java_path(self): On 2014/12/18 22:30:47, rothwell wrote: > maybe this should be a classmethod as it modifies class state Removed. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:123: """Changes a line number from /tmp/file:300 to /orig/file:100. On 2014/12/18 22:30:47, rothwell wrote: > maybe elaborate on why this conversion is necessary? why would the line numbers > in the tmp file not match the original? Done. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:173: (exitcode, output) The exit code of the Closure compiler (as a number) On 2014/12/18 22:30:47, rothwell wrote: > is exitcode useful to the caller? I would expect a bool to make more sense. Done. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:186: return On 2014/12/18 22:30:47, rothwell wrote: > this return code doesn't match your stated API Done. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:191: rel_path = lambda f: os.path.join(os.path.relpath(os.getcwd(), tmp_dir), f) On 2014/12/18 22:30:47, rothwell wrote: > not sure this lambda is buying you much, I would just move this into the list > comprehension I find: includes = [rel_path(f) for f in depends + [source_file]] more readable than: includes = [os.path.join(os.path.relpath(os.getcwd(), tmp_dir), f) for f in depends + [source_file]] do you disagree? https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:231: return bool(errors), output On 2014/12/18 22:30:47, rothwell wrote: > your API says you will return an integer, this is a boolean Done. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:252: exit, _ = checker.check(source, depends=depends, externs=externs) On 2014/12/18 22:30:48, rothwell wrote: > why return an error string if it's just going to be ignored? it is used here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur... but these tests aren't passing anyways, so Done. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:261: open(opts.out_file, "w").write("") On 2014/12/18 22:30:47, rothwell wrote: > it seems confusing to write an empty file - shouldn't this feature just be > unimplemented for now, rather than incompletely implemented? this empty file is used by the build system for incremental build purposes (e.g. if the source file is newer than this empty file, recompile). added more doc. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... File third_party/closure_compiler/processor.py (right): https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/processor.py:24: class FileCache(object): On 2014/12/18 22:30:48, rothwell wrote: > It doesn't look like this needs to be a class, it can just be a function. what would I do with _cache? make it a global? I'd prefer it to be private to a class that cares about it. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/processor.py:78: def __init__(self, source_file): On 2014/12/18 22:30:48, rothwell wrote: > docstring - e.g. what is a source_file, a filename or a file object Done. https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/processor.py:91: continue # Stay on the same line. ^ this is why I can't use enumerate(), because a line needs to be re-processed after it's changed https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/processor.py:123: def included_files(self): On 2014/12/18 22:30:48, rothwell wrote: > if you're trying to obscure access to the underlying data structure this should > return a copy of the list. otherwise you should just make included_files a > public field Done. (pydoc doesn't show these properties for me now, though...)
ping rothwell@
ping rothwell@
The CQ bit was checked by rothwell@google.com
lgtm https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/476453002/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:191: rel_path = lambda f: os.path.join(os.path.relpath(os.getcwd(), tmp_dir), f) On 2015/02/25 03:36:49, Dan Beam wrote: > On 2014/12/18 22:30:47, rothwell wrote: > > not sure this lambda is buying you much, I would just move this into the list > > comprehension > > I find: > > includes = [rel_path(f) for f in depends + [source_file]] > > more readable than: > > includes = [os.path.join(os.path.relpath(os.getcwd(), tmp_dir), f) > for f in depends + [source_file]] > > do you disagree? defer to your preference https://codereview.chromium.org/476453002/diff/220001/third_party/closure_com... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/476453002/diff/220001/third_party/closure_com... third_party/closure_compiler/checker.py:96: """Logs |msg| to stdout if --verbose/-v is passed when invoking this script. is this syntax, |x| common in chromium?
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/476453002/220001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM 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.
dbeam@chromium.org changed reviewers: + thestig@chromium.org
+thestig@ for chrome committer stamp https://codereview.chromium.org/476453002/diff/220001/third_party/closure_com... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/476453002/diff/220001/third_party/closure_com... third_party/closure_compiler/checker.py:96: """Logs |msg| to stdout if --verbose/-v is passed when invoking this script. On 2015/03/20 19:24:31, rothwell wrote: > is this syntax, |x| common in chromium? yes, |var_names| or |_member_vars| or anything in monospace font in docs (is basically my interpretation).
Patchset #4 (id:240001) has been deleted
rs lgtm
Patchset #4 (id:260001) has been deleted
The CQ bit was checked by dbeam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rothwell@google.com, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/476453002/#ps280001 (title: "remove space")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/476453002/280001
Message was sent while issue was closed.
Committed patchset #4 (id:280001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c5503ce046fdad8c76129c0061f0d6011c2fe92b Cr-Commit-Position: refs/heads/master@{#321693}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:280001) has been created in https://codereview.chromium.org/1023253003/ by pfeldman@chromium.org. The reason for reverting is: http://build.chromium.org/p/chromium.chromiumos/buildstatus?builder=Linux%20C....
Message was sent while issue was closed.
It was not the best name for the change as well.
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:280001) has been created in https://codereview.chromium.org/1022293002/ by thakis@chromium.org. The reason for reverting is: Looks like this broke the Linux and Linux 64 clobber bots: http://build.chromium.org/p/chromium/builders/Linux%20x64/builds/433 http://build.chromium.org/p/chromium/builders/Linux/builds/59857 FAILED: cd ../../remoting; python ../third_party/closure_compiler/checker.py --strict --no-single-file --success-stamp ../out/Release/remoting_key_tester_jscompile.stamp tools/javascript_key_tester/background.js tools/javascript_key_tester/chord_tracker.js tools/javascript_key_tester/event_listeners.js tools/javascript_key_tester/main.js webapp/js_proto/chrome_proto.js webapp/js_proto/chrome_event_proto.js (ERROR) Exception in thread "main" java.lang.UnsupportedClassVersionError: org/chromium/closure/compiler/Runner : Unsupported major.minor version 51.0 at java.lang.ClassLoader.defineClass1(Native Method) at java.lang.ClassLoader.defineClassCond(ClassLoader.java:631) at java.lang.ClassLoader.defineClass(ClassLoader.java:615) at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:141) at java.net.URLClassLoader.defineClass(URLClassLoader.java:283) at java.net.URLClassLoader.access$000(URLClassLoader.java:58) at java.net.URLClassLoader$1.run(URLClassLoader.java:197) at java.security.AccessController.doPrivileged(Native Method) at java.net.URLClassLoader.findClass(URLClassLoader.java:190) at java.lang.ClassLoader.loadClass(ClassLoader.java:306) at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:301) at java.lang.ClassLoader.loadClass(ClassLoader.java:247) Could not find the main class: org.chromium.closure.compiler.Runner. Program will exit..
fwiw: I hunted down the cause of issues that led to reverting and am fixing them here: https://codereview.chromium.org/1037613002/ this CL actually exposed that remoting/ was compiling JS in a relatively undefined Java environment and due to an unfortunate bug (that I fixed here) Java stacks weren't being shown/changing the exit code of checker.py tl;dr - I'll reland this as-is when it's safe and I'm glad we found these bugs.
The CQ bit was checked by dbeam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/476453002/280001
Message was sent while issue was closed.
Committed patchset #4 (id:280001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/999677e0674bf4dae5e4a0b03e25947d6d8a844d Cr-Commit-Position: refs/heads/master@{#322255} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
