1018096 - Clean up pyflakes, pylint and pep8 warnings

Status: RESOLVED FIXED

(Testing :: mozregression, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Let's update mozregression to comply with pyflakes, pylint, and pep8.

Comment 1

[Julien Pagès (:parkouss)]

Attachment: coding style fixes

There are big changes to review !

We discussed with Dave Hunt about this bug, and he told me to ask you for a review.

The aim is to clean a bit the code, so we can more easily refactor it and be able to reuse the logic of mozregression in b2ghaystack.

The next steps are already marked with the following bugs:

- bug 1018098
- bug 1018101

In this patch, I did not change any public name (function, methods, named arguments and instance variables).
Dave asked to keep compatibility with consumers of mozregression.

The patch is also currently viewable on my github account ([1]), and I can send a pull request if you prefer.

I ran pylint on the patched code, so I'm pretty sure it won't break anything. But there are no unittests, so a carefull review is required here.

[1] https://github.com/parkouss/mozregression/compare/mozilla:master...master

Comment 2

[William Lachance (:wlach)]

Comment on attachment 8431533 [details] [diff] [review]
coding style fixes

Review of attachment 8431533 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Nice to get this cleaned up. Some minor requests for changes, could you upload a new patch? (a pull request would be fine too)

::: mozregression/regression.py
@@ +10,5 @@
>  from optparse import OptionParser
> +from inboundfinder import getInboundRevisions
> +from runnightly import NightlyRunner, parseBits
> +from runinbound import InboundRunner
> +from utils import strsplit, get_date

When you change get_date in utils you'll need to update this as well.

@@ +81,3 @@
>          options += ['exit']
>          while verdict not in options:
> +            verdict = raw_input("Was this %s build good, bad, or broken? (type 'good', 'bad', 'skip', 'retry', or 'exit' and press Enter): " % buildType)

I think I preferred it before this change. This is kind of unreasonably long for a one-liner.

@@ +219,3 @@
>  def cli():
>      parser = OptionParser()
> +    parser.add_option("-b", "--bad", dest="bad_date",help="first known bad nightly build, default is today",

Please keep help= on a seperate line.

We want to keep lines to 80 characters where reasonable.

@@ +224,2 @@
>                        metavar="YYYY-MM-DD", default=None)
> +    parser.add_option("-e", "--addons", dest="addons",help="list of addons to install", metavar="PATH1,PATH2", default="")

As above, please keep help= on a seperate line.

@@ +224,4 @@
>                        metavar="YYYY-MM-DD", default=None)
> +    parser.add_option("-e", "--addons", dest="addons",help="list of addons to install", metavar="PATH1,PATH2", default="")
> +    parser.add_option("-p", "--profile", dest="profile", help="profile to use with nightlies", metavar="PATH")
> +    parser.add_option("-a", "--args", dest="cmdargs", help="command-line arguments to pass to the application",

As above, please keep help= on a seperate line.

@@ +228,2 @@
>                        metavar="ARG1,ARG2", default="")
> +    parser.add_option("-n", "--app", dest="app", help="application name (firefox, fennec or thunderbird)",

As above, please keep help= on a seperate line.

@@ +228,4 @@
>                        metavar="ARG1,ARG2", default="")
> +    parser.add_option("-n", "--app", dest="app", help="application name (firefox, fennec or thunderbird)",
> +                      metavar="[firefox|fennec|thunderbird]", default="firefox")
> +    parser.add_option("-r", "--repo", dest="repo_name", help="repository name on ftp.mozilla.org",

As above, please keep help= on a seperate line.

@@ +234,5 @@
> +                      choices=("32","64"), default=mozinfo.bits)
> +    parser.add_option("--persist", dest="persist", help="the directory in which files are to persist ie. /Users/someuser/Documents")
> +    parser.add_option("--inbound", action="store_true", dest="inbound", help="use inbound instead of nightlies (use --good-rev and --bad-rev options")
> +    parser.add_option("--bad-rev", dest="firstBadRevision",help="first known bad revision (use with --inbound)")
> +    parser.add_option("--good-rev", dest="lastGoodRevision",help="last known good revision (use with --inbound)")

As above, please keep help= on a seperate line for all of these.

::: mozregression/runinbound.py
@@ +1,1 @@
> +from utils import urlLinks, strsplit, get_date

When you change get_date in utils you'll need to update this as well.

@@ +76,5 @@
>                        metavar="PATH1,PATH2")
> +    parser.add_option("-p", "--profile", dest="profile", help="path to profile to user", metavar="PATH")
> +    parser.add_option("--bits", dest="bits", help="force 32 or 64 bit version (only applies to x86_64 boxes)",
> +                      choices=("32","64"), default=mozinfo.bits)
> +    parser.add_option("--persist", dest="persist", help="the directory in which files are to persist ie. /Users/someuser/Documents")

Please keep help= on seperate lines for these.

::: mozregression/runnightly.py
@@ +111,4 @@
>              raise NotImplementedError("Can't invoke abstract base class")
>          self.remove_tempdir()
>          self.tempdir = tempfile.mkdtemp()
> +        self.binary = mozinstall.get_binary(mozinstall.install(src=self.dest, dest=self.tempdir), self.name)

Seems a bit long for one line, can you move it back to the way it was before or at least make it squeeze into 80 characters?

::: mozregression/utils.py
@@ +13,5 @@
>  def strsplit(string, sep):
>      # XXX https://github.com/mozilla/mozregression/issues/50
>      strlist = string.split(sep)
>      if strlist == ['']:
> +      return []

This looks like a mistake? 4 space -> 2 space indentation?

@@ +18,3 @@
>      return strlist
>  
> +def get_date(dateString):

Could you rename this to getDate?

@@ +27,3 @@
>  
>  def update_download_progress(percent):
> +    sys.stdout.write("===== Downloaded %d%% =====\r"%percent)

This also looks like a mistake? The spacing before made things clearer. :)

@@ +48,3 @@
>          dest = os.path.basename(url)
>  
> +    f = open(tmp_file, 'wb')

It would probably be cleared to do something like:

with tempfile.NamedTemporaryFile() as f:
   ...
   os.rename(f.name, dest)

Then we could omit the f.close() at the end, while ensuring that things were cleaned up. Not to mention get rid of temp_file altogether. :)

See: https://docs.python.org/2/library/tempfile.html#tempfile.NamedTemporaryFile

@@ +70,3 @@
>          return []
>  
> +    soup = BeautifulSoup(content)

I don't think we need the temporary "content" variable here. just BeautifulSoup(r.text) seems fine.

Comment 3

[Julien Pagès (:parkouss)]

Attachment: fixing coding style

I'm really sorry William, I made a mistake.

I sended you the reverse patch! it is the exact opposite of what I have done. I'm really sorry for that, but I'm new to git and inversed the command.

I used:

git diff HEAD..mozilla/master > 1018096.patch

instead of:

git diff mozilla/master..HEAD > 1018096.patch

I was wondering why you were talking about line too long, as a I had corrected them !

Comment 4

[William Lachance (:wlach)]

Comment on attachment 8431626 [details] [diff] [review]
fixing coding style

Ah... so actually that makes most of your variable changes not right. :( Since the dominant style in mozregression is camel case (fooBar) we should keep that consistent throughout. 

Either that or *convert* everything to use underscores (you'd probably want to use a macro for that like this: http://stackoverflow.com/questions/1175208/elegant-python-function-to-convert-camelcase-to-camel-case) -- although that has a good risk of breaking stuff so it's probably not worthwhile.

Could you submit a new version of the patch without the variable/function naming changes?

Comment 5

[Julien Pagès (:parkouss)]

Mmh.In fact, I previously renamed all to use underscores, as recommended by pep8.

But then Dave told me that to ensure compatibility with other projects we had to kept the public names - so I made another commit to revert some of my work.

But maybe there are no other projects using mozregression internal api, so we could just adopt pep8 naming convention ?

You can look at the diff [1]. It renaming all with underscore is OK for you, then I will revert my last commit and send you a patch / pull request with the work already done visible in [1].

[1] https://github.com/parkouss/mozregression/compare/mozilla:master...c85deadaa3f646996f803221d2877c7bc073c68e

Comment 6

[William Lachance (:wlach)]

(In reply to j.parkouss from comment #5)
> Mmh.In fact, I previously renamed all to use underscores, as recommended by
> pep8.
> 
> But then Dave told me that to ensure compatibility with other projects we
> had to kept the public names - so I made another commit to revert some of my
> work.
> 
> But maybe there are no other projects using mozregression internal api, so
> we could just adopt pep8 naming convention ?

Yeah, I don't think anything uses the mozregression public api. If you really want to convert the whole codebase to underscored variable names, I wouldn't object. We'll just need to make sure that it all still works.

Comment 7

[Julien Pagès (:parkouss)]

I understand. Unit tests would have been usefull here !

Pylint is good at finding bugs like "Undefined variable %r" or "No name %r in module %r" and the like. So I'm pretty confident.

I created a pull request ([1]).

[1] https://github.com/mozilla/mozregression/pulls

Comment 8

[William Lachance (:wlach)]

This looks great! I need to do some testing first but I think a rebased version of this patch can definitely go into mozregression.

Comment 9

[William Lachance (:wlach)]

Ok, did some testing on windows and linux. There was a minor problem with some rewriting of utils.py, where we renamed a variable CHUNK to chunk caused a bunch of problems (since we were already using a variable called chunk for something else). Renaming chunk to chunk_size fixed that. Otherwise everything seems to work as before.

For future reference, just running something like:

mozregression -g 2014-03-10 -b 2014-03-12

and then marking builds as bad for a while should test the key codepaths (i.e. does mozregression still work for nightly and inbound builds)

I'm just going to push an amended, rebased patch as I'd like Julien to start hacking on some more interesting bugs. :)

https://github.com/mozilla/mozregression/commit/3cd4d366ad53389b39a264c120fc6c3820978e05

Sam: Heads up, this changes the coding style in a bunch of places in mozregression. You might need to rebase / adjust your patches.