Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9323016:
[WebUI] Add some presubmit checks (Closed)
|
Created:
8 years, 10 months ago by Dan Beam Modified:
8 years, 10 months ago CC:
chromium-reviews, Dirk Pranke Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[WebUI] Add some presubmit checks to make WebUI resources follow the
Chromium "Web Development Style Guide".
<http://www.chromium.org/developers/web-development-style-guide>;
R=estade@chromium.org,maruel@chromium.org,jhawkins@chromium.org
BUG=None
TEST=python chrome/browser/resources/PRESUBMIT.py and run automatically when the file's in the CL.
NOTRY=true
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=122054
Patch Set 1 #Patch Set 2 : fixing hex values and other stuff #Patch Set 3 : fixes #
Total comments: 10
Patch Set 4 : moving, tweaking, still much to do #Patch Set 5 : most rules working well, need to find home for tests (and write them) #
Total comments: 8
Patch Set 6 : review comments and tests #
Total comments: 3
Patch Set 7 : review comments from estade and tbreisacher #Patch Set 8 : fixed false negative, changed dir handling #
Total comments: 1
Patch Set 9 : modularizing #
Total comments: 8
Patch Set 10 : remove debug messages #
Total comments: 11
Patch Set 11 : review comments #
Total comments: 7
Patch Set 12 : review comments #
Total comments: 2
Patch Set 13 : try: finally: around path modifier #Patch Set 14 : adding find_depot_tools #
Total comments: 4
Patch Set 15 : unfiltering .html and .js #Patch Set 16 : some pylint nits #
Messages
Total messages: 36 (0 generated)
http://codereview.chromium.org/9323016/diff/3/presubmit_canned_checks.py File presubmit_canned_checks.py (right): http://codereview.chromium.org/9323016/diff/3/presubmit_canned_checks.py#newc... presubmit_canned_checks.py:875: resources = lambda x: input_api.FilterSourceFile(x, black_list=black_list, # TODO(dbeam): add more folders to this list http://codereview.chromium.org/9323016/diff/3/presubmit_canned_checks.py#newc... presubmit_canned_checks.py:908: snapshot("checking resouces") sp
wait for just a little while on this, maruel@ and estade@ http://codereview.chromium.org/9323016/diff/3/presubmit_canned_checks.py File presubmit_canned_checks.py (right): http://codereview.chromium.org/9323016/diff/3/presubmit_canned_checks.py#newc... presubmit_canned_checks.py:875: resources = lambda x: input_api.FilterSourceFile(x, black_list=black_list, On 2012/02/03 04:13:15, Evan Stade wrote: > # TODO(dbeam): add more folders to this list Will do soon, this is kind of a hacky version (should be using os.path.join()) just to get it working, but I'll add that before it goes in. http://codereview.chromium.org/9323016/diff/3/presubmit_canned_checks.py#newc... presubmit_canned_checks.py:908: snapshot("checking resouces") On 2012/02/03 04:13:15, Evan Stade wrote: > sp Thank you.
http://codereview.chromium.org/9323016/diff/3/presubmit_canned_checks.py File presubmit_canned_checks.py (right): http://codereview.chromium.org/9323016/diff/3/presubmit_canned_checks.py#newc... presubmit_canned_checks.py:7: import re No need, input_api.re http://codereview.chromium.org/9323016/diff/3/presubmit_canned_checks.py#newc... presubmit_canned_checks.py:876: white_list='.*chrome/browser/resources/(?:options2|ntp4).*\.(?j:cs)s$') This doesn't belong to canned checks. Then if should be in the relevant to PRESUBMIT.py. http://codereview.chromium.org/9323016/diff/3/presubmit_canned_checks.py#newc... presubmit_canned_checks.py:930: def _CheckWebDevStyleGuide(input_api, output_api, source_file_filter): why the underscore?
https://chromiumcodereview.appspot.com/9323016/diff/3/presubmit_canned_checks.py File presubmit_canned_checks.py (right): https://chromiumcodereview.appspot.com/9323016/diff/3/presubmit_canned_checks... presubmit_canned_checks.py:7: import re On 2012/02/03 16:41:35, Marc-Antoine Ruel wrote: > No need, input_api.re Ah, OK, will do this in new file. https://chromiumcodereview.appspot.com/9323016/diff/3/presubmit_canned_checks... presubmit_canned_checks.py:876: white_list='.*chrome/browser/resources/(?:options2|ntp4).*\.(?j:cs)s$') On 2012/02/03 16:41:35, Marc-Antoine Ruel wrote: > This doesn't belong to canned checks. Then if should be in the relevant to > PRESUBMIT.py. Yeah, I figured this yesterday (that this wasn't the right place), will put in chrome/browser/resources/PRESUBMIT.py https://chromiumcodereview.appspot.com/9323016/diff/3/presubmit_canned_checks... presubmit_canned_checks.py:930: def _CheckWebDevStyleGuide(input_api, output_api, source_file_filter): On 2012/02/03 16:41:35, Marc-Antoine Ruel wrote: > why the underscore? It was just to match local style (all the other functions in this file are doing the same).
https://chromiumcodereview.appspot.com/9323016/diff/5/chrome/browser/resource... File chrome/browser/resources/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/9323016/diff/5/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:13: import os Please use input_api.os_path and input_api.re instead. https://chromiumcodereview.appspot.com/9323016/diff/5/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:20: Please use 2 vertical spaces between file level symbols. https://chromiumcodereview.appspot.com/9323016/diff/5/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:73: lines = _remove_comments(input_api.ReadFile(f[0], 'rb')).splitlines() This pattern is really slow. You are reading the same file 8 times! Use NewContents(). https://chromiumcodereview.appspot.com/9323016/diff/5/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:187: for f in filter(lambda l: l[1].endswith('.css'), files): Why not do instead ar: checks = [ list of lambda to check a single line ] for f in filter(lambda l: l[1].endswith('.css'), files): for line in _remove_comments(f.NewContents()).splitlines(): for check in checks: check(f, line) I'd probably be several times faster.
ptal, added tests and rewrote a bit of the checks https://chromiumcodereview.appspot.com/9323016/diff/5/chrome/browser/resource... File chrome/browser/resources/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/9323016/diff/5/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:13: import os On 2012/02/06 20:40:51, Marc-Antoine Ruel wrote: > Please use input_api.os_path and input_api.re instead. Done. https://chromiumcodereview.appspot.com/9323016/diff/5/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:20: On 2012/02/06 20:40:51, Marc-Antoine Ruel wrote: > Please use 2 vertical spaces between file level symbols. Done. https://chromiumcodereview.appspot.com/9323016/diff/5/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:73: lines = _remove_comments(input_api.ReadFile(f[0], 'rb')).splitlines() On 2012/02/06 20:40:51, Marc-Antoine Ruel wrote: > This pattern is really slow. You are reading the same file 8 times! Use > NewContents(). Cool, I was waiting until this was slow to optimize but if this already exists all the better, :) https://chromiumcodereview.appspot.com/9323016/diff/5/chrome/browser/resource... chrome/browser/resources/PRESUBMIT.py:187: for f in filter(lambda l: l[1].endswith('.css'), files): On 2012/02/06 20:40:51, Marc-Antoine Ruel wrote: > Why not do instead ar: > > checks = [ > list of lambda to check a single line > ] > > for f in filter(lambda l: l[1].endswith('.css'), files): > for line in _remove_comments(f.NewContents()).splitlines(): > for check in checks: > check(f, line) > > I'd probably be several times faster. Ended up doing something like that.
Isn't https://chromiumcodereview.appspot.com/9288045/ a superset of this CL?
On 2012/02/08 14:31:47, Marc-Antoine Ruel wrote: > Isn't https://chromiumcodereview.appspot.com/9288045/ a superset of this CL? No, it's JavaScript, this is CSS, but whoever lands after (probably me) will need to merge with the other, for sure.
+tbreisacher for review given his (convincingly similar, :D) work.
ping
Logic lgtm; will wait on Marc-Antoine for final LG.
https://chromiumcodereview.appspot.com/9323016/diff/13002/chrome/browser/reso... File chrome/browser/resources/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/9323016/diff/13002/chrome/browser/reso... chrome/browser/resources/PRESUBMIT.py:208: for f in filter(lambda l: l[0].endswith('.css'), files): nit: Lowercase l looks like numeral 1. You could use 'line' or something.
http://codereview.chromium.org/9323016/diff/13002/chrome/browser/resources/PR... File chrome/browser/resources/PRESUBMIT.py (right): http://codereview.chromium.org/9323016/diff/13002/chrome/browser/resources/PR... chrome/browser/resources/PRESUBMIT.py:208: for f in filter(lambda l: l[0].endswith('.css'), files): On 2012/02/09 02:46:07, Tyler Breisacher wrote: > nit: Lowercase l looks like numeral 1. You could use 'line' or something. Yeah, I'll remove any use of l, that's a bad var name (there's a whole school of people that hate l).
estade: fixed 80 char wrap just for you. http://codereview.chromium.org/9323016/diff/13002/chrome/browser/resources/PR... File chrome/browser/resources/PRESUBMIT.py (right): http://codereview.chromium.org/9323016/diff/13002/chrome/browser/resources/PR... chrome/browser/resources/PRESUBMIT.py:208: for f in filter(lambda l: l[0].endswith('.css'), files): On 2012/02/09 02:46:07, Tyler Breisacher wrote: > nit: Lowercase l looks like numeral 1. You could use 'line' or something. Done.
http://codereview.chromium.org/9323016/diff/19002/chrome/browser/resources/PR... File chrome/browser/resources/PRESUBMIT.py (right): http://codereview.chromium.org/9323016/diff/19002/chrome/browser/resources/PR... chrome/browser/resources/PRESUBMIT.py:15: from testing_support.super_mox import SuperMoxTestBase, mox As I said before, I dislike that; presubmit_support.py and trychange.py are loading all the relevant PRESUBMIT.py in their process. Having super_mox loaded in an executable could have unforeseen side effects. Let's say in 2 years from now, once alzheimer kicks in, an unrelated change in super_mox.py does a side on import, mocking something important. That would break this presubmit check and this would be really hard to debug. That's my main point here, I don't want to debug this kind of problem in 2 years from now. I assure you, debugging mock left overs isn't fun. So use input_api.canned_checks.RunUnitTests( input_api, output_api, [input_api.os_path.join(input_api.PresubmitLocalPath(), 'test_presubmit.py')]) and have test_presubmit.py do whatever it wants. It's fine at that point to mock everything since it's out of process.
https://chromiumcodereview.appspot.com/9323016/diff/20002/chrome/browser/reso... File chrome/browser/resources/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/9323016/diff/20002/chrome/browser/reso... chrome/browser/resources/PRESUBMIT.py:39: except ImportError: I'd do it unconditionally to reduce variation. https://chromiumcodereview.appspot.com/9323016/diff/20002/chrome/browser/reso... File chrome/browser/resources/test_presubmit.py (right): https://chromiumcodereview.appspot.com/9323016/diff/20002/chrome/browser/reso... chrome/browser/resources/test_presubmit.py:260: print "Got here!" Eh. https://chromiumcodereview.appspot.com/9323016/diff/20002/chrome/browser/reso... File chrome/browser/resources/web_dev_style/__init__.py (right): https://chromiumcodereview.appspot.com/9323016/diff/20002/chrome/browser/reso... chrome/browser/resources/web_dev_style/__init__.py:7: __all__ = ['css_checker'] A 0 byte file should be fine. https://chromiumcodereview.appspot.com/9323016/diff/20002/chrome/browser/reso... File chrome/browser/resources/web_dev_style/css_checker.py (right): https://chromiumcodereview.appspot.com/9323016/diff/20002/chrome/browser/reso... chrome/browser/resources/web_dev_style/css_checker.py:15: class CSSChecker(): class CSSChecker(object): https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... File chrome/browser/resources/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... chrome/browser/resources/PRESUBMIT.py:29: if presubmit in [f.AbsoluteLocalPath() for f in input_api.AffectedFiles()]: if presubmit in (f.AbsoluteLocalPath() for f in input_api.AffectedFiles()): is marginally better. I would have preferred looking for LocalPath() but I'm not sure there's a good way to calculate that. :/ https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... chrome/browser/resources/PRESUBMIT.py:39: sys.path.append(resources) sys.path.insert(0, resources) https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... File chrome/browser/resources/test_presubmit.py (right): https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... chrome/browser/resources/test_presubmit.py:12: from testing_support.super_mox import SuperMoxTestBase ROOT = os.path.dirname(os.path.abspath(__file__)) sys.insert(0, ROOT)
https://chromiumcodereview.appspot.com/9323016/diff/20002/chrome/browser/reso... File chrome/browser/resources/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/9323016/diff/20002/chrome/browser/reso... chrome/browser/resources/PRESUBMIT.py:39: except ImportError: On 2012/02/09 22:14:14, Marc-Antoine Ruel wrote: > I'd do it unconditionally to reduce variation. Done. https://chromiumcodereview.appspot.com/9323016/diff/20002/chrome/browser/reso... File chrome/browser/resources/test_presubmit.py (right): https://chromiumcodereview.appspot.com/9323016/diff/20002/chrome/browser/reso... chrome/browser/resources/test_presubmit.py:260: print "Got here!" On 2012/02/09 22:14:14, Marc-Antoine Ruel wrote: > Eh. All removed in next upload, :D https://chromiumcodereview.appspot.com/9323016/diff/20002/chrome/browser/reso... File chrome/browser/resources/web_dev_style/__init__.py (right): https://chromiumcodereview.appspot.com/9323016/diff/20002/chrome/browser/reso... chrome/browser/resources/web_dev_style/__init__.py:7: __all__ = ['css_checker'] On 2012/02/09 22:14:14, Marc-Antoine Ruel wrote: > A 0 byte file should be fine. I was using from web_dev_style import * in the test_presubmit.py, guess I can add when there's more than 1 module. https://chromiumcodereview.appspot.com/9323016/diff/20002/chrome/browser/reso... File chrome/browser/resources/web_dev_style/css_checker.py (right): https://chromiumcodereview.appspot.com/9323016/diff/20002/chrome/browser/reso... chrome/browser/resources/web_dev_style/css_checker.py:15: class CSSChecker(): On 2012/02/09 22:14:14, Marc-Antoine Ruel wrote: > class CSSChecker(object): Done. (removed ()) https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... File chrome/browser/resources/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... chrome/browser/resources/PRESUBMIT.py:29: if presubmit in [f.AbsoluteLocalPath() for f in input_api.AffectedFiles()]: On 2012/02/09 22:14:14, Marc-Antoine Ruel wrote: > if presubmit in (f.AbsoluteLocalPath() for f in input_api.AffectedFiles()): > > is marginally better. Done. (list -> tuple) > I would have preferred looking for LocalPath() but I'm not sure there's a good > way to calculate that. :/ I think you saw why it's hard-ish to do that from tbreisacher's code, yeah? https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... chrome/browser/resources/PRESUBMIT.py:39: sys.path.append(resources) On 2012/02/09 22:14:14, Marc-Antoine Ruel wrote: > sys.path.insert(0, resources) Done. https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... File chrome/browser/resources/test_presubmit.py (right): https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... chrome/browser/resources/test_presubmit.py:12: from testing_support.super_mox import SuperMoxTestBase On 2012/02/09 22:14:14, Marc-Antoine Ruel wrote: > ROOT = os.path.dirname(os.path.abspath(__file__)) > sys.insert(0, ROOT) This doesn't work so far as SuperMoxTestBase is in depot_tools, not here. Still working on how I'd actually add this to the path when running from RunUnitTests(). I'm thinking I'm going to have to pass the cwd() from depot_tools on PYTHONPATH to the new script.
You forgot to upload. https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... File chrome/browser/resources/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... chrome/browser/resources/PRESUBMIT.py:29: if presubmit in [f.AbsoluteLocalPath() for f in input_api.AffectedFiles()]: On 2012/02/14 17:02:01, Dan Beam wrote: > On 2012/02/09 22:14:14, Marc-Antoine Ruel wrote: > > if presubmit in (f.AbsoluteLocalPath() for f in input_api.AffectedFiles()): > > > > is marginally better. > > Done. (list -> tuple) You are confusing tuple and generators. This is a generator, not a tuple. And yes this is confusing that both are using parenthesis. https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... File chrome/browser/resources/test_presubmit.py (right): https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... chrome/browser/resources/test_presubmit.py:12: from testing_support.super_mox import SuperMoxTestBase On 2012/02/14 17:02:01, Dan Beam wrote: > On 2012/02/09 22:14:14, Marc-Antoine Ruel wrote: > > ROOT = os.path.dirname(os.path.abspath(__file__)) > > sys.insert(0, ROOT) > > This doesn't work so far as SuperMoxTestBase is in depot_tools, not here. Still > working on how I'd actually add this to the path when running from > RunUnitTests(). I'm thinking I'm going to have to pass the cwd() from > depot_tools on PYTHONPATH to the new script. I often use an adhoc script to do it automatically, search for find_depot_tools.py.
https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... File chrome/browser/resources/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... chrome/browser/resources/PRESUBMIT.py:29: if presubmit in [f.AbsoluteLocalPath() for f in input_api.AffectedFiles()]: On 2012/02/14 17:15:19, Marc-Antoine Ruel wrote: > On 2012/02/14 17:02:01, Dan Beam wrote: > > On 2012/02/09 22:14:14, Marc-Antoine Ruel wrote: > > > if presubmit in (f.AbsoluteLocalPath() for f in input_api.AffectedFiles()): > > > > > > is marginally better. > > > > Done. (list -> tuple) > > You are confusing tuple and generators. This is a generator, not a tuple. And > yes this is confusing that both are using parenthesis. See updated code (and tell me if it's still wrong). https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... File chrome/browser/resources/test_presubmit.py (right): https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... chrome/browser/resources/test_presubmit.py:12: from testing_support.super_mox import SuperMoxTestBase On 2012/02/14 17:15:19, Marc-Antoine Ruel wrote: > On 2012/02/14 17:02:01, Dan Beam wrote: > > On 2012/02/09 22:14:14, Marc-Antoine Ruel wrote: > > > ROOT = os.path.dirname(os.path.abspath(__file__)) > > > sys.insert(0, ROOT) > > > > This doesn't work so far as SuperMoxTestBase is in depot_tools, not here. > Still > > working on how I'd actually add this to the path when running from > > RunUnitTests(). I'm thinking I'm going to have to pass the cwd() from > > depot_tools on PYTHONPATH to the new script. > > I often use an adhoc script to do it automatically, search for > find_depot_tools.py. But this is script is also in depot_tools. You're saying run this function before calling external tests in RunUnitTest()? https://chromiumcodereview.appspot.com/9323016/diff/24001/chrome/browser/reso... File chrome/browser/resources/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/9323016/diff/24001/chrome/browser/reso... chrome/browser/resources/PRESUBMIT.py:29: if presubmit in (f.AbsoluteLocalPath() for f in input_api.AffectedFiles()): it's a tuple here
https://chromiumcodereview.appspot.com/9323016/diff/24001/chrome/browser/reso... File chrome/browser/resources/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/9323016/diff/24001/chrome/browser/reso... chrome/browser/resources/PRESUBMIT.py:29: if presubmit in (f.AbsoluteLocalPath() for f in input_api.AffectedFiles()): On 2012/02/14 17:48:46, Dan Beam wrote: > it's a tuple here or a generator or something, whatever you told me to write
> You forgot to upload. No coffee yet, uploaded now.
On 2012/02/14 17:48:46, Dan Beam wrote: > > I often use an adhoc script to do it automatically, search for > > find_depot_tools.py. > > But this is script is also in depot_tools. You're saying run this function > before calling external tests in RunUnitTest()? See http://src.chromium.org/viewvc/chrome/trunk/tools/commit-queue/find_depot_too... and http://src.chromium.org/viewvc/chrome/trunk/tools/commit-queue/commit_queue.p... for corresponding examples. Also, remove the print __file__ https://chromiumcodereview.appspot.com/9323016/diff/24001/chrome/browser/reso... File chrome/browser/resources/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/9323016/diff/24001/chrome/browser/reso... chrome/browser/resources/PRESUBMIT.py:29: if presubmit in (f.AbsoluteLocalPath() for f in input_api.AffectedFiles()): On 2012/02/14 17:49:30, Dan Beam wrote: > On 2012/02/14 17:48:46, Dan Beam wrote: > > it's a tuple here > > or a generator or something, whatever you told me to write Read the rationale of http://www.python.org/dev/peps/pep-0289/ https://chromiumcodereview.appspot.com/9323016/diff/24001/chrome/browser/reso... chrome/browser/resources/PRESUBMIT.py:35: sys.path.insert(0, resources) you need to use a try/finally here. https://chromiumcodereview.appspot.com/9323016/diff/24001/chrome/browser/reso... File chrome/browser/resources/web_dev_style/css_checker.py (right): https://chromiumcodereview.appspot.com/9323016/diff/24001/chrome/browser/reso... chrome/browser/resources/web_dev_style/css_checker.py:15: class CSSChecker: I had meant textually: class CSSChecker(object): See http://docs.python.org/reference/datamodel.html#new-style-and-classic-classes
https://chromiumcodereview.appspot.com/9323016/diff/24001/chrome/browser/reso... File chrome/browser/resources/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/9323016/diff/24001/chrome/browser/reso... chrome/browser/resources/PRESUBMIT.py:35: sys.path.insert(0, resources) On 2012/02/14 18:03:36, Marc-Antoine Ruel wrote: > you need to use a try/finally here. You told me to remove for simplicity (http://goo.gl/4vR6E), right? Or do you mean around sys.path.insert()? https://chromiumcodereview.appspot.com/9323016/diff/24001/chrome/browser/reso... File chrome/browser/resources/web_dev_style/css_checker.py (right): https://chromiumcodereview.appspot.com/9323016/diff/24001/chrome/browser/reso... chrome/browser/resources/web_dev_style/css_checker.py:15: class CSSChecker: On 2012/02/14 18:03:36, Marc-Antoine Ruel wrote: > I had meant textually: > class CSSChecker(object): > > See > http://docs.python.org/reference/datamodel.html#new-style-and-classic-classes Done.
https://chromiumcodereview.appspot.com/9323016/diff/12004/chrome/browser/reso... File chrome/browser/resources/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/9323016/diff/12004/chrome/browser/reso... chrome/browser/resources/PRESUBMIT.py:35: sys.path.insert(0, resources) I had meant to call sys.path.insert() all the time, not only on exception like you did before. But you still need to keep a try/finally including all the lines up to line 46.
https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... File chrome/browser/resources/test_presubmit.py (right): https://chromiumcodereview.appspot.com/9323016/diff/22002/chrome/browser/reso... chrome/browser/resources/test_presubmit.py:12: from testing_support.super_mox import SuperMoxTestBase On 2012/02/14 17:15:19, Marc-Antoine Ruel wrote: > On 2012/02/14 17:02:01, Dan Beam wrote: > > On 2012/02/09 22:14:14, Marc-Antoine Ruel wrote: > > > ROOT = os.path.dirname(os.path.abspath(__file__)) > > > sys.insert(0, ROOT) > > > > This doesn't work so far as SuperMoxTestBase is in depot_tools, not here. > Still > > working on how I'd actually add this to the path when running from > > RunUnitTests(). I'm thinking I'm going to have to pass the cwd() from > > depot_tools on PYTHONPATH to the new script. > > I often use an adhoc script to do it automatically, search for > find_depot_tools.py. Done. (this script is in in tools/, I was on crack) https://chromiumcodereview.appspot.com/9323016/diff/12004/chrome/browser/reso... File chrome/browser/resources/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/9323016/diff/12004/chrome/browser/reso... chrome/browser/resources/PRESUBMIT.py:35: sys.path.insert(0, resources) On 2012/02/14 22:15:09, Marc-Antoine Ruel wrote: > I had meant to call sys.path.insert() all the time, not only on exception like > you did before. > But you still need to keep a try/finally including all the lines up to line 46. Done.
lgtm
https://chromiumcodereview.appspot.com/9323016/diff/14003/chrome/browser/reso... File chrome/browser/resources/web_dev_style/css_checker.py (right): https://chromiumcodereview.appspot.com/9323016/diff/14003/chrome/browser/reso... chrome/browser/resources/web_dev_style/css_checker.py:71: return line.find('"') >= 0 the argument is "lines" but then you use "line" in the body. https://chromiumcodereview.appspot.com/9323016/diff/14003/chrome/browser/reso... chrome/browser/resources/web_dev_style/css_checker.py:178: for f in filter(lambda f: f[0].endswith('.css'), files): I think you've already filtered this to check only CSS files, in PRESUBMIT.py
https://chromiumcodereview.appspot.com/9323016/diff/14003/chrome/browser/reso... File chrome/browser/resources/web_dev_style/css_checker.py (right): https://chromiumcodereview.appspot.com/9323016/diff/14003/chrome/browser/reso... chrome/browser/resources/web_dev_style/css_checker.py:71: return line.find('"') >= 0 On 2012/02/15 01:22:41, Tyler Breisacher wrote: > the argument is "lines" but then you use "line" in the body. Thank you for this catch. line existed in the previous scope, so this would've been a real bitch to find. Changed. https://chromiumcodereview.appspot.com/9323016/diff/14003/chrome/browser/reso... chrome/browser/resources/web_dev_style/css_checker.py:178: for f in filter(lambda f: f[0].endswith('.css'), files): On 2012/02/15 01:22:41, Tyler Breisacher wrote: > I think you've already filtered this to check only CSS files, in PRESUBMIT.py Yeah, I shouldn't be, so I added .js and .html to the allowed files in PRESUBMIT.py and left this check as is so it filter()s down to only what it needs (and the same results can be passed to each check). The reason I want this is that someday there may be .html files that have .js and .css we can parse (though there really isn't much currently), but each test should be passed a list of all the relevant resource files and it should be able to do what it wants/need with them. Make sense?
On 2012/02/15 01:39:53, Dan Beam wrote: > Make sense? Yup. We also might want to do some style-checking on the HTML itself someday. LGTM and I'll do the same thing on the JS side :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dbeam@chromium.org/9323016/27001
Presubmit check for 9323016-27001 failed and returned exit status 1. warning: code.google.com certificate with fingerprint ce:3e:bd:64:4d:e5:66:1c:e8:63:eb:99:3d:7e:75:36:a0:86:0e:03 not verified (check hostfingerprints or web.cacerts config setting) Running presubmit commit checks ... ************* Module test_presubmit F0401: 21,0: Unable to import 'web_dev_style' C0324:216,4:WebDevStyleGuideTest.testCssZeroLengthTerms: Comma not followed by a space self.VerifyContentsProducesOutput(""" }""",""" ^^ W0611: 19,0: Unused import find_depot_tools ************* Module css_checker W0622: 44,26:CSSChecker.RunChecks.alphabetize_props: Redefining built-in 'file' W0622: 90,30:CSSChecker.RunChecks.one_selector_per_line: Redefining built-in 'file' Checking out rietveld... Running push-basic.sh Running save-description-on-failure.sh Running basic.sh Running upload-stale.sh Running upload-local-tracking-branch.sh Running patch.sh Running submit-from-new-dir.sh Running post-dcommit-hook-test.sh Running hooks.sh Running abandon.sh Running upstream.sh ** Presubmit Messages ** You should install python 2.5 and run ln -s $(which python2.5) python. A great place to put this symlink is in depot_tools. Otherwise, you break depot_tools on python 2.5, you get to keep the pieces. ** Presubmit ERRORS ** Fix pylint errors first. /mnt/data/b/commit-queue/workdir/tools/depot_tools/chrome/browser/resources/test_presubmit.py failed! Command /mnt/data/b/commit-queue/workdir/tools/depot_tools/chrome/browser/resources/test_presubmit.py returned non-zero exit status 1 in /mnt/data/b/commit-queue/workdir/tools/depot_tools/chrome/browser/resources Traceback (most recent call last): File "/mnt/data/b/commit-queue/workdir/tools/depot_tools/chrome/browser/resources/test_presubmit.py", line 19, in <module> import find_depot_tools ImportError: No module named find_depot_tools Presubmit checks took 391.2s to calculate.
On 2012/02/15 01:53:14, I haz the power (commit-bot) wrote: > Presubmit check for 9323016-27001 failed and returned exit status 1. > > warning: http://code.google.com certificate with fingerprint > ce:3e:bd:64:4d:e5:66:1c:e8:63:eb:99:3d:7e:75:36:a0:86:0e:03 not verified (check > hostfingerprints or web.cacerts config setting) > Running presubmit commit checks ... > ************* Module test_presubmit > F0401: 21,0: Unable to import 'web_dev_style' Use # pylint: disable=F0401 > C0324:216,4:WebDevStyleGuideTest.testCssZeroLengthTerms: Comma not followed by a > space > self.VerifyContentsProducesOutput(""" > }""",""" > ^^ Put a space. > W0611: 19,0: Unused import find_depot_tools Use # pylint: disable=W0611 > ************* Module css_checker > W0622: 44,26:CSSChecker.RunChecks.alphabetize_props: Redefining built-in 'file' > W0622: 90,30:CSSChecker.RunChecks.one_selector_per_line: Redefining built-in > 'file' Change the variable name to f.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dbeam@chromium.org/9323016/15006
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dbeam@chromium.org/9323016/15006
Change committed as 122054 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
