Closed
Bug 1149691
Opened 9 years ago
Closed 9 years ago
make the code more compliant to pep8 using flake8
Categories
(Testing :: Talos, defect)
Testing
Talos
Tracking
(firefox40 affected)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox40 | --- | affected |
People
(Reporter: parkouss, Assigned: parkouss)
Details
Attachments
(1 file, 2 obsolete files)
36.59 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
flake8 can help to write code that will looks the same everywhere by conforming to some styling convention. Also it often helps to clean the code, by pointing out unused variables/imports, and can even detect some real bugs. For now there are a lot of output if we run "flake talos", so we can not see important things - let's clean this out. Another step may be to run flake8 automatically before (or after if we can't afford before) each commit. This can help to review and to be sure new code looks consistent.
Comment 1•9 years ago
|
||
For the pep8 parts, I'd highly recommend using autopep8 to speed up the changes, since it can fix a large proportion of the pep8 warnings automatically.
Assignee | ||
Comment 2•9 years ago
|
||
Thanks Ed! I'll try that. :)
Assignee | ||
Comment 3•9 years ago
|
||
So, quite some work here. I hope this will work. I mixed manual fixing with autopep8 automatic cleaning (for example to fix indentation). :jmaher, I feel sorry for this review! Don't hate me. :) The good thing is that I removed some unused imports, variables, and even the method FFSetup.install_addon which seems to be unseless now (probably replaced with mozprofile api). Well, we'll see if that pass! Else I suggest to split the patch, and try to merge those patches one after the other. Talos seems to be runing fine locally.
Attachment #8612575 -
Flags: review?(jmaher)
Comment 4•9 years ago
|
||
Comment on attachment 8612575 [details] [diff] [review] make the code more compliant to pep8 using flake8 Review of attachment 8612575 [details] [diff] [review]: ----------------------------------------------------------------- actually I just have a bunch of small nits. ::: talos/TalosProcess.py @@ +20,5 @@ > """ > + def __init__(self, cmd, args=None, cwd=None, env=None, > + ignore_children=False, logfile=None, > + suppress_javascript_errors=False, wait_for_quit_timeout=5, > + **kwargs): I liked the previous version better. ::: talos/compare.py @@ +28,5 @@ > +branch_map['Beta'] = {'pgo': {'id': 53, 'name': 'Mozilla-Beta'}} > +branch_map['Cedar'] = {'pgo': {'id': 26, 'name': 'Cedar'}, > + 'nonpgo': {'id': 26, 'name': 'Cedar'}} > +branch_map['UX'] = {'pgo': {'id': 59, 'name': 'UX'}, > + 'nonpgo': {'id': 137, 'name': 'UX-Non-PGO'}} the previous version was nicer for this map layout. ::: talos/ffprocess.py @@ +79,4 @@ > scheme = "http://" > + if server.startswith('http://') or \ > + server.startswith('chrome://') or \ > + server.startswith('file:///'): the indentation here is not pretty compared to before. ::: talos/post_file.py @@ +47,5 @@ > for url in urls: > url_split = urlparse.urlsplit(url) > scheme, server, path, _, _ = url_split > + if scheme in ('http', 'https') and \ > + not link_exists(server, path, scheme): I don't like the indentation here. ::: talos/results.py @@ +84,5 @@ > _output = output.GraphserverOutput(self) > results = _output() > + _output.output('file://%s' % os.path.join(os.getcwd(), > + 'results.out'), > + results) this doesn't look quite right- maybe put the os.path.join on a new line? ::: talos/run_tests.py @@ +80,5 @@ > > scheme = "http://" > + if webserver.startswith('http://') or \ > + webserver.startswith('chrome://') or \ > + webserver.startswith('file:///'): the original spacing was much nicer. ::: talos/startup_test/media/media_utils.py @@ +49,1 @@ > all of the stuff above here was much nicer before. ::: talos/talosconfig.py @@ +21,5 @@ > browser_config['command'] = ' '.join(command_line) > > + if ('xperf_providers' in test_config) and \ > + ('xperf_user_providers' in test_config) and \ > + ('xperf_stackwalk' in test_config): the indentation here is confusing compared to before. ::: talos/test.py @@ +334,5 @@ > sps_profile_interval = 10 > sps_profile_entries = 1000000 > + win_counters = w7_counters = linux_counters = mac_counters = \ > + remote_counters = None > + """ ASAP mode this is odd, we indent the remote_counters = None part. then add the comment below and it reads awkward. @@ +367,5 @@ > tpmozafterpaint = False > sps_profile_interval = 1 > sps_profile_entries = 10000000 > + win_counters = w7_counters = linux_counters = mac_counters = \ > + remote_counters = None same thing here. ::: talos/ttest.py @@ +310,3 @@ > if test_config['name'] != 'tprovider': > + utils.setEnvironmentVars( > + {'MOZ_DISABLE_NONLOCAL_CONNECTIONS': '1'}) we could probably remove this hack now that we don't run tprovider anymore! @@ +525,5 @@ > print stdout > print stderr > print "end of setup command output" > + print ("---------------------------------------" > + "--------------------------------------") lets just remove the block of code to print this out, it was for debugging purposes more than production. (lines 522-529 in the new file) @@ +630,5 @@ > pass > try: > os.remove(os.path.join(profile_dir, 'sessionstore.bak')) > except: > pass it would be nice to see: for name in ['sessionstore.js', '.parentlock', 'sessionstore.bak']: try: os.remove(...) except: pass
Attachment #8612575 -
Flags: review?(jmaher) → review+
Comment 5•9 years ago
|
||
hey, try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=de67619f5256
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #5) > hey, try looks good: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=de67619f5256 Awesome! I'll look at your comments to fix the patch early next week.
Assignee | ||
Comment 7•9 years ago
|
||
So I started to look at this. I can see that you don't like the extra indentation for multiline if statements. :) This is right following pep8 convention: http://legacy.python.org/dev/peps/pep-0008/#multiline-if-statements But I agree this is not the first recommendation in this official doc. The problem is that the pep8/flake8tools seems to have a bug when the indentation is the same, even if I add a blank line or a comment line: talos/ffprocess.py:82:13: E129 visually indented line with same indent as next logical line While we must not be annoyed with bugs from a tool that is here to help us, I really like the idea that running 'flake8 talos' does not output warnings. This way we could automate it, and not look at the results thinking "oh, this warning is normal, this one is not". I propose that we talk more about it on irc - I'm not against your point of view, and there are a few ways we could take to make us both happy. :)
Comment 8•9 years ago
|
||
(In reply to Julien Pagès from comment #7) > While we must not be annoyed with bugs from a tool that is here to help us, > I really like the idea that running 'flake8 talos' does not output warnings. > This way we could automate it, and not look at the results thinking "oh, > this warning is normal, this one is not". This x 1000. Note you can always ignore certain warning types in a setup.cfg or tox.ini in the root of the repo. Also there are often several ways of fixing the same warning - autopep8 only picks one (and unless you enforce a very large max line length often the wrong one).
Assignee | ||
Comment 9•9 years ago
|
||
I fixed a few nits - but not all yet. This should be better anyway.
Attachment #8613498 -
Flags: review+
Comment 10•9 years ago
|
||
landed latest patch: http://hg.mozilla.org/build/talos/rev/4a71115d7367 this failed for: talos/profile/symbolication.py talos/talosconfig.py I didn't want to hack on these as I assumed a script did most of the work. Lets update to the latest code and try again. We can address those files along with remaining comments in the next patch (on this bug)
Assignee | ||
Comment 11•9 years ago
|
||
Next step for the pep8 fixes. I tried to fix what you told me in talos/ttest.py, hopefuly this will work (I made some assumptions here). The last patch error you had is probably related to the fact that I have converted those files to linux EOL, and copy pasted the patch content. This time I attached the patch, we'll see if that works.
Attachment #8612575 -
Attachment is obsolete: true
Attachment #8613498 -
Attachment is obsolete: true
Attachment #8613522 -
Flags: review?(jmaher)
Comment 12•9 years ago
|
||
Comment on attachment 8613522 [details] [diff] [review] 1149691-2.patch Review of attachment 8613522 [details] [diff] [review]: ----------------------------------------------------------------- this is great and I like the general cleanup that happened as well.
Attachment #8613522 -
Flags: review?(jmaher) → review+
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•