Closed Bug 1149691 Opened 9 years ago Closed 9 years ago

make the code more compliant to pep8 using flake8

Categories

(Testing :: Talos, defect)

defect
Not set
normal

Tracking

(firefox40 affected)

RESOLVED FIXED
Tracking Status
firefox40 --- affected

People

(Reporter: parkouss, Assigned: parkouss)

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
Thanks Ed! I'll try that. :)
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 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+
(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.
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. :)
(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).
I fixed a few nits - but not all yet. This should be better anyway.
Attachment #8613498 - Flags: review+
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)
Attached patch 1149691-2.patchSplinter Review
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 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+
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.

Attachment

General

Created:
Updated:
Size: