Solve all flake8 warnings #163
Conversation
I've checked with pep8 too: now whipper is fully PEP8 compliant. Implemented changes proposed by Freso.
whipper/common/encode.py
Outdated
| @@ -65,6 +67,8 @@ def _flac_encode(self): | |||
| # I only made it a task for now because that it's easier to integrate in | |||
| # program/cdparanoia.py - where whipper currently does the tagging. | |||
| # We should just move the tagging to a more sensible place. | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
I still think the comments should move "into" the class they're commenting on, rather than being separated further from it. Does flake8 also complain if the comments are inside the class?
whipper/image/toc.py
Outdated
| # of current track as parsed so far reset on each TRACK statement | ||
| currentLength = 0 | ||
| totalLength = 0 # accrued during TRACK record parsing, total disc | ||
| pregapLength = 0 # length of the pre-gap, current track in for loop |
There was a problem hiding this comment.
I also still think it would make more sense to put the comments before the assignments here, esp. considering the long comment for relativeOffset.
whipper/test/common.py
Outdated
| @@ -68,14 +68,15 @@ def readCue(self, name): | |||
| version so we can use it in comparisons. | |||
| """ | |||
| ret = open(os.path.join(os.path.dirname(__file__), name)).read( | |||
| ).decode('utf-8') | |||
| ).decode('utf-8') | |||
There was a problem hiding this comment.
Maybe instead do something like:
cuefile = os.path.join(os.path.dirname(__file__), name)
ret = open(cuefile).read().decode('utf-8')This still splits it up in two lines, but instead of continuing one line's logic into the next, it makes the two lines have distinct pieces of the logic, making it easier to follow along in the code.
There was a problem hiding this comment.
(Same as you did with filename in some of the tests further down.)
whipper/test/test_common_config.py
Outdated
| @@ -21,23 +21,25 @@ def tearDown(self): | |||
|
|
|||
| def testAddReadOffset(self): | |||
| self.assertRaises(KeyError, | |||
| self._config.getReadOffset, 'PLEXTOR ', 'DVDR PX-L890SA', '1.05') | |||
| self._config.getReadOffset, 'PLEXTOR ', | |||
There was a problem hiding this comment.
I would move 'PLEXTOR ', to the next line and maybe move self._config.… to the line above (if that still stays within the limit) or just keep that on its own line. It's nicer on the (well, my :)) eyes to have the drive info together.
whipper/test/test_common_config.py
Outdated
| self.assertEquals(offset, 6) | ||
|
|
||
| def testAddReadOffsetSpaced(self): | ||
| self.assertRaises(KeyError, | ||
| self._config.getReadOffset, 'Slimtype', 'eSAU208 2 ', 'ML03') | ||
| self._config.getReadOffset, 'Slimtype', |
There was a problem hiding this comment.
Same as the testAddReadOffset() comment.
Second round of improvements suggested by Freso.
I've checked with pep8 too: now whipper is fully PEP8 compliant.
Implemented changes proposed by Freso.