Solve all flake8 warnings #158

JoeLametta · 2017-05-10T14:07:15Z

I've checked with pep8 too: now whipper is fully PEP8 compliant

JoeLametta · 2017-05-10T14:11:08Z

I'm not very confident with Travis CI: should we remove the allow_failures from .travis.yml now that whipper is fully PEP8 compliant?

Freso · 2017-05-16T12:58:46Z

I'm not very confident with Travis CI: should we remove the allow_failures from .travis.yml now that whipper is fully PEP8 compliant?

That would be my recommendation, yes. :) I made a PR for this PR here: #160 ;)

Freso

Lots and lots of comments. Overall, I think this is by far a major improvement to whipper's code, however, I do have a (pet?) peeves. Namely the line length limitation. It's great when sticking to 79 characters/line make sense, but sometimes setting that up as a hard limit ultimately results in the code being harder to read, not easier. I would suggested either increasing our limit (flake8 can be called with --max-line-lengh=…) or ignoring line length altogether (--ignore=E501).

Also, there is some old commented-out code that is being "fixed". It should just get removed entirely—we're already using a VCS (git), so there's no need to keep old code around like this.

And lastly, it seems there are some things related to pycdio/the import of pycdio that are completely unrelated to PEP8/flake8? They should probably at the very least go into their own commit, but probably into a PR of their own. (Esp. considering how invasive the flake8 changes already are on their own.)

Freso · 2017-05-16T13:02:19Z

whipper/command/debug.py

@@ -244,7 +242,8 @@ def do(self):
 sys.stdout.write('- Release %d:\n' % (i + 1, ))
 sys.stdout.write(' Artist: %s\n' % md.artist.encode('utf-8'))
 sys.stdout.write(' Title: %s\n' % md.title.encode('utf-8'))
- sys.stdout.write(' Type: %s\n' % md.releaseType.encode('utf-8'))
+ sys.stdout.write(' Type: %s\n' %
+ md.releaseType.encode('utf-8'))


I would personally prefer to not split this line and instead tell flake8 to ignore line-length errors. (Or give it a longer line-length.)

This change, IMO, makes the code less beautiful and harder to read: Without the line break, this statement looks exactly like the ones above and below it, with the line break, it now visually appears different from the surrounding but actually functionally identical statements.

Freso · 2017-05-16T13:03:21Z

whipper/command/drive.py

 class Analyze(BaseCommand):
 summary = "analyze caching behaviour of drive"
- description = """Determine whether cdparanoia can defeat the audio cache of the drive."""
+ description = """Determine whether cdparanoia can defeat \
+ the audio cache of the drive."""


Same thing here. I would probably not split this line (but I would probably replace """…""" with just "…").

Freso · 2017-05-16T13:06:16Z

whipper/command/drive.py

@@ -50,15 +52,20 @@ def do(self):
 'cdparanoia can defeat the audio cache on this drive.\n')

 info = drive.getDeviceInfo(self.options.device)
+ # TODO: As pycdio is a hard dependency for whipper
+ # the code should be updated to reflect this: acting nicely
+ # when it isn't available isn't needed anymore


Great to have TODO comments, but they really should be added as separate commit(s). :D </pedantic>

Also, for maximum PEP8 compliance (even if flake8 doesn't catch this), the comment should have proper grammar (IIRC). In this case, it's missing a period at the end. ;) </pedantic-for-realz>

Freso · 2017-05-16T13:07:27Z

whipper/command/drive.py

- sys.stdout.write('Drive caching behaviour not saved: could not get device info (requires pycdio).\n')
+ sys.stdout.write(
+ 'Drive caching behaviour not saved: could not get '
+ 'device info (requires pycdio).\n')


I would maybe split after the saved: instead, to make the text flow better. (But I didn't check whether this would make it fit within the 79 character limit… but again, I don't think we need strict adherence to that one.)

Freso · 2017-05-16T13:08:09Z

whipper/command/drive.py

- sys.stdout.write(
- 'Install pycdio for vendor/model/release detection.\n')
- return
-


Is this a flake8 fix, or related to the TODO comment above? If the latter, it should definitely be in its own commit.

Freso · 2017-05-16T13:57:33Z