Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add flake8 to CI #355

Closed
ilevkivskyi opened this issue Jan 17, 2017 · 21 comments
Closed

Add flake8 to CI #355

ilevkivskyi opened this issue Jan 17, 2017 · 21 comments

Comments

@ilevkivskyi
Copy link
Member

I just tried to run flake8 locally and I have got around hundred errors. I think it is a good idea to add flake8 to CI. I would like to make a PR fixing those errors and adding flake8 to .travis.yml.

@gvanrossum, is it better to do this before or after other open PRs are merged?
@gvanrossum
Copy link
Member

@ambv Any suggestions here? I'm a bit worried that such a PR would cause merge conflicts for every other PR currently in flight...

@ambv
Copy link
Contributor

ambv commented Jan 18, 2017 •
edited
Loading

Looks reasonable to me.

Open questions

@gvanrossum, what will happen if some core dev modifies typing.py right in the cpython repo? Do we sync that back here? Or do you revert those changes in the cpython repo? Shouldn't that stop happening since typing is no longer provisional? That part of the workflow is "TBD" in the README.

I'm asking those questions mostly in the context of flake8. If people in the cpython repo can change the module, they should run the same checks as here.

The second reason I'm asking is that if there's changes in cpython that are not here, this will create ugly conflict on that other repo's side. I can check for that.

Assuming we decide introducing the tool doesn't interfere with core development workflow, it's relatively trivial to introduce formatting changes incrementally (per flake8 rule) so that history looks sensible and it doesn't conflict.

There's just one PR in flight. Is it ready for merging? Looks pretty ready to me. I wouldn't worry about stuff that isn't submitted against us yet.

Existing situation and ways forward

These are the violation statistics for typing.py, not too bad:

19    E501 line too long
5     E129 visually indented line with same indent as next logical line
4     E305 expected 2 blank lines after class or function definition, found 1
4     E302 expected 2 blank lines, found 1
4     E128 continuation line under-indented for visual indent
3     W503 line break before binary operator
2     E711 comparison to None should be 'if cond is None:'
1     E306 expected 1 blank line before a nested definition, found 0
1     E271 multiple spaces after keyword
1     E261 at least two spaces before inline comment
1     B007 Loop control variable 'i' not used within the loop body. If this is intended, start the name with an underscore.

Most of lines too long are reasonable as they are, the longest is 97 characters, most are below 85. I'm a little reluctant to touch those without a more important reason, what do you think @gvanrossum? Blank lines, however, are trivial to fix without conflicts. And others look like easy isolated fixes to make, too.

test_typing.py is more laborious:

48    E306 expected 1 blank line before a nested definition, found 0
42    E701 multiple statements on one line (colon)
41    F821 undefined name 'ann_module', etc.
25    E501 line too long (82 > 79 characters)
10    E704 multiple statements on one line (def)
4     F841 local variable 'joe' is assigned to but never used
2     E302 expected 2 blank lines, found 1
2     E231 missing whitespace after ','
2     E128 continuation line under-indented for visual indent

The tricky one is F821. That's one of the most useful warnings but in this case it's fooled by PY36_TESTS being included as a string for backwards compatibility. Ignoring that warning would be a shame. So, we could separate those to a file of its own and import them or provide dummy values for older Python versions.

I'd ignore E701 and E704 since the most common pattern in tests is a harmless line like: class C: .... I'd ignore lines that are too long. Rest is trivial to fix.

@gvanrossum
Copy link
Member

what will happen if some core dev modifies typing.py right in the cpython repo?

I prefer it if core devs make changes here first, but not all of them do that, so I catch those and merge them back here. I also sometimes have to ensure that Python 3.5, 3.6 and 3.7 have the same exact text (everything needs to be dynamically checking for versions).

Anyway, right now there are no discrepancies. I always manually check before running the update-stdlib.sh script.

FWIW typing.py is still provisional in 3.6 (you're maybe confused with asyncio, which is not).

I am not worried about people who have unsubmitted changes to typing.py. There aren't many people who contribute, and we always steer them here.

@ambv
Copy link
Contributor

ambv commented Jan 18, 2017

From What's New in 3.6:

The typing module received a number of improvements and is no longer provisional.

The module also doesn't have the provisional preamble in the documentation for 3.6.

Alright, given the rest of your comment, I'll start cleaning things up. Will submit PRs for semantic changes but won't bother you with whitespace-only manipulations.

@gvanrossum
Copy link
Member

gvanrossum commented Jan 18, 2017 via email

@gvanrossum
Copy link
Member

gvanrossum commented Jan 18, 2017 via email

@ambv
Copy link
Contributor

ambv commented Jan 18, 2017

Looking at #247, it seems the last major piece left is structural types, something I will work on for Python 3.7 (you can count on that). That should be a separate PEP at this point anyway. We can experiment with it in mypy_extensions for the time being.

All other things are relatively minor, except for AsyncGenerator which I'm a little sad was missed from 3.6.0.

@ilevkivskyi ilevkivskyi mentioned this issue Jan 18, 2017
Merged
@ilevkivskyi
Copy link
Member Author

@ambv

There's just one PR in flight. Is it ready for merging? Looks pretty ready to me. I wouldn't worry about stuff that isn't submitted against us yet.

It is merged now. You can go ahead with your changes, whenever you want.

All other things are relatively minor, except for AsyncGenerator which I'm a little sad was missed from 3.6.0.

To be honest, I see the omission of AsyncGenerator as a bug, not as an absence of feature. So that I believe it could be added already in 3.6.1 (not in 3.7.0).

Looking at #247, it seems the last major piece left is structural types, something I will work on for Python 3.7 (you can count on that)

I am also interested in working on protocols. Do you have more concrete plans? I was thinking about polishing what Jukka proposes in #11 (comment) into a PEP, and implementing it in mypy. In parallel, I am going to start next week with implementing structural checks for __call__ against Callable[[t1, t2, ...], tr]. See python/mypy#797

@ambv
Copy link
Contributor

ambv commented Jan 18, 2017 •
edited
Loading

Do you have more concrete plans?

The reason I didn't want us to reinvent the wheel too quickly is that I'd like to research ways in which ABCs and zope.interface don't fit the bill. The former is a workable workaround for many cases today, whereas the latter seems like a superset of what we need to me. Preparing a totally new way to do this without drawing from those two experiences seemed irresponsible to me.

BTW, it was the same with @overload having a runtime effect in .py files (effectively multiple dispatch). I'm happy how this was implemented in the end, having it purely for typing purposes is brilliant.

@ilevkivskyi
Copy link
Member Author

@ambv

I'm happy how this was implemented in the end, having it purely for typing purposes is brilliant.

I think this should be done the same way for protocols. The only runtime effect is that ProtocolMeta subclasses GenericMeta so that protocols will be normal ABCs, no other runtime effects, no fancy isinstance or issubclass checks etc. The only purpose of protocol specification is for mypy and other static type checkers.

I agree that we should incorporate previous experience. Do you think it would be good idea to start from Jukka's proposal (I was thinking about it for some time and I really like it), or you would like to propose something completely different?

OK, probably this is off-topic so I stop disturbing you here. (If you are interested in the discussion we could move it to #11)

@ambv
Copy link
Contributor

ambv commented Jan 18, 2017

OK, probably this is off-topic so I stop disturbing you here.

Haha, that's right, it is. FWIW Jukka's proposal is a very reasonable starting point but before we commit to it, let's look closer at existing runtime tools.

@ambv
Copy link
Contributor

ambv commented Jan 18, 2017

Four pull requests here are related to enabling flake8: #357, #358, #359, #360.

@ambv
Copy link
Contributor

ambv commented Jan 18, 2017

The checks are already running on Travis (with Python 3.6), I pushed the config change along with trivial whitespace changes in 5ed8c89.

@ilevkivskyi
Copy link
Member Author

All the four PRs LGTM.

@gvanrossum
Copy link
Member

gvanrossum commented Jan 19, 2017 via email

@ilevkivskyi
Copy link
Member Author

I merged the PRs. What are we going to do with test_typing.py and long lines (E501)?

@gvanrossum
Copy link
Member

gvanrossum commented Jan 19, 2017 via email

@ilevkivskyi
Copy link
Member Author

The longest line is already 97 chars now. So that the only problem left is F821 undefined name in test_typing.py because we use exec() for 3.5 and 3.6. Should we just ignore those or add dummy values as @ambv proposed?

@ambv
Copy link
Contributor

ambv commented Jan 19, 2017

Thanks for acting quickly on this :)

Let's not ignore F821, I rather decided to exclude test_* entirely than to ignore undefined names in the actual module. So, let's put some dummy values for test_*. There's some other issues there to be solved, I'll send you a batch of new PRs for those.

@ambv
Copy link
Contributor

ambv commented Jan 20, 2017

This is pretty much done. Tests have their specific config because of four remaining warnings:

91    E306 expected 1 blank line before a nested definition, found 0
81    E701 multiple statements on one line (colon)
12    E704 multiple statements on one line (def)
10    F811 redefinition of unused 'A' from line 90

To my eye, all of them are harmless for tests but worth checking for the actual module, hence the separate config.

Feel free to adjust the configs to your liking from now on.

@ambv ambv closed this as completed Jan 20, 2017
@ilevkivskyi
Copy link
Member Author

@ambv This all makes sense. Thank you for helping with this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ambv @gvanrossum @ilevkivskyi