Skip to content

Add flake8 to CI #355

Closed
Closed
Add flake8 to CI#355

Description

@ilevkivskyi
ilevkivskyi
opened on Jan 17, 2017

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?

Activity

  1. gvanrossum commented on Jan 17, 2017

    @gvanrossum
    gvanrossum
    on Jan 17, 2017
    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...

  2. ambv commented on Jan 18, 2017

    @ambv
    ambv
    on Jan 18, 2017
    Contributor

    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.

  3. gvanrossum commented on Jan 18, 2017

    @gvanrossum
    gvanrossum
    on Jan 18, 2017
    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.

  4. ambv commented on Jan 18, 2017

    @ambv
    ambv
    on Jan 18, 2017
    Contributor

    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.

  5. gvanrossum commented on Jan 18, 2017

    @gvanrossum
    gvanrossum
    on Jan 18, 2017 via email
    Member

  6. gvanrossum commented on Jan 18, 2017

    @gvanrossum
    gvanrossum
    on Jan 18, 2017 via email
    Member

  7. ambv commented on Jan 18, 2017

    @ambv
    ambv
    on Jan 18, 2017
    Contributor

    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.

  8. ilevkivskyi commented on Jan 18, 2017

    @ilevkivskyi
    ilevkivskyi
    on Jan 18, 2017
    MemberAuthor

    @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

  9. ambv commented on Jan 18, 2017

    @ambv
    ambv
    on Jan 18, 2017
    Contributor

    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.

  10. ilevkivskyi commented on Jan 18, 2017

    @ilevkivskyi
    ilevkivskyi
    on Jan 18, 2017
    MemberAuthor

    @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)

  11. ambv commented on Jan 18, 2017

    @ambv
    ambv
    on Jan 18, 2017
    Contributor

    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.

  12. ambv commented on Jan 18, 2017

    @ambv
    ambv
    on Jan 18, 2017
    Contributor

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

  13. ambv commented on Jan 18, 2017

    @ambv
    ambv
    on Jan 18, 2017
    Contributor

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

  14. ilevkivskyi commented on Jan 18, 2017

    @ilevkivskyi
    ilevkivskyi
    on Jan 18, 2017
    MemberAuthor

    All the four PRs LGTM.

  15. gvanrossum commented on Jan 19, 2017

    @gvanrossum
    gvanrossum
    on Jan 19, 2017 via email
    Member

  16. ilevkivskyi commented on Jan 19, 2017

    @ilevkivskyi
    ilevkivskyi
    on Jan 19, 2017
    MemberAuthor

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

  17. gvanrossum commented on Jan 19, 2017

    @gvanrossum
    gvanrossum
    on Jan 19, 2017 via email
    Member

  18. ilevkivskyi commented on Jan 19, 2017

    @ilevkivskyi
    ilevkivskyi
    on Jan 19, 2017
    MemberAuthor

    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?

  19. ambv commented on Jan 19, 2017

    @ambv
    ambv
    on Jan 19, 2017
    Contributor

    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.

  20. ambv commented on Jan 20, 2017

    @ambv
    ambv
    on Jan 20, 2017
    Contributor

    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.

  21. ambv
    closed this as completedon Jan 20, 2017

  22. ilevkivskyi commented on Jan 20, 2017

    @ilevkivskyi
    ilevkivskyi
    on Jan 20, 2017
    MemberAuthor

    @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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions