Add flake8 linting #351
Add flake8 linting #351
Conversation
|
To avoid making this PR bigger than it already is, I left a couple flake8 plugins commented out in the dependencies. I'll open a separate PR enabling those since they're going to require a lot of shuffling/tweaking code. Side note: Is |
|
I'm in 2 minds about this. flake8 does catch a few things which you could call bugs but they are mostly minor. But it does generate a lot of false positives forcing you carefully exclude those false positives. In my experience flake8 can be a neutral or even negative influence on code quality, if you have good code coverage. And it is far inferior to mypy (which will really come in to its own when we drop Py27). That said, I do like that you've been pragmatic and configured it appropriately, so I'm willing to give it a go. Just a few questions in the PR... |
| @@ -34,7 +34,8 @@ def _to_str(size, suffixes, base): | |||
| elif size < base: | |||
| return "{:,} bytes".format(size) | |||
|
|
|||
| for i, suffix in enumerate(suffixes, 2): | |||
| # TODO (dargueta): Don't rely on unit or suffix being defined in the loop. | |||
willmcgugan
Aug 31, 2019
Member
As long as there is at least one value in suffixes we can rely on those variables being defined. Perhaps an assert len(suffixes) is appropriate?
As long as there is at least one value in suffixes we can rely on those variables being defined. Perhaps an assert len(suffixes) is appropriate?
dargueta
Sep 1, 2019
Author
Contributor
That'd work.
That'd work.
dargueta
Sep 1, 2019
•
Author
Contributor
So actually I found an edge case -- this will break for values above 1024 bytes (which is absurdly high) because it'll run off the edge of the array and unit will never be defined.
While incredibly unlikely to be encountered in practice, a troll could still use this to break things. I've modified it to both get rid of the linter warning and throw a helpful error message in the event this gets handed a huge number.
So actually I found an edge case -- this will break for values above 1024 bytes (which is absurdly high) because it'll run off the edge of the array and unit will never be defined.
While incredibly unlikely to be encountered in practice, a troll could still use this to break things. I've modified it to both get rid of the linter warning and throw a helpful error message in the event this gets handed a huge number.
willmcgugan
Sep 1, 2019
Member
I don't see how it will break. It will use the last unit, which may result in an absurdly large value, bit it's still valid.
>>> from fs.filesize import traditional
>>> traditional(1E32)
'82,718,061.3 YB'
>>> traditional(1E64)
'8,271,806,125,530,276,925,072,459,481,976,663,965,696.0 YB'
I don't see how it will break. It will use the last unit, which may result in an absurdly large value, bit it's still valid.
>>> from fs.filesize import traditional
>>> traditional(1E32)
'82,718,061.3 YB'
>>> traditional(1E64)
'8,271,806,125,530,276,925,072,459,481,976,663,965,696.0 YB'
dargueta
Sep 1, 2019
Author
Contributor
Odd...broke for me. o.O
Odd...broke for me. o.O
dargueta
Sep 2, 2019
Author
Contributor
I removed the change btw
I removed the change btw
Interesting! I'm curious to hear more. I chose flake8 over pylint because, although pylint is much better at catching logic bugs, it can get super annoying. flake8 also has a lot of nifty plugins. |
|
Oh also -- for the openers, is there a reason why we import the file system classes inside the opener function? Circular dependencies or something? |
|
@dargueta It may be a cultural thing, but orgs I've worked for wanted to stick ruthlessly to flake8 and discouraged exceptions of any kind. Which meant that sometimes you modify good code for zero benefit (possibly even making it worse). But I could come round to a more pragmatic use of flake8. re the openers. Memory is a bit hazy, but I think that was more to do with reducing the time to do |
|
@willmcgugan I could see that you are not convinced whether flake8 would result in good code. As I mentioned about unused imports, dead variables in other issues, they are caught at design time when flake8 is integrated with IDE. Each line that flake8 flags up indicates some underlying issue. Hence, when an alien python file is opened, one would automatically look for red lines. All these issues are very cheap to identify.
Well, I think this is not related to flake8 introduction. Any other code change may result in unwanted changes. I would argue why unit tests, integration tests would fail to warn the new developer who would break good code. The fear that unwanted code change, which usually is due to code refactoring, will break good code exists in all software projects, is affecting all software developers and dominates many code review discussions. Unless we face it, it will hunt us forever. My advice is to fight with it with more tests, whichever reproducible way to test our fear. Then back to flak8 discussion, because it is cheap to find issues at design time, it is also cheap to get rid of at design time with TDD. And we keep such effort on daily basis so that the code will not smell like a still pond. |
Oh yeah, I see how that'd be a problem. I am all up for disabling warnings when they interfere with productivity and readability, but warnings about bare |
|
Thanks @dargueta ! |
Type of changes
Checklist
Description
Bugs (or potential ones)
getattrcall that would cause an AttributeError where it was supposed to prevent it.except:withexcept Exception:, see here for why.Cleanup
if Falsewithif typing.TYPE_CHECKINGfrom *with explicit imports in some testsOther
@overloadis also ignored, not just@typing.overloadExpansion on discussion in #332.