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.

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

Fix flake8 issues - #2581 #2582

Merged
merged 43 commits into from Jul 19, 2017
Merged

Conversation

@andras-tim
Copy link
Contributor

@andras-tim andras-tim commented Jul 17, 2017

Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs:

  • Add a new news fragment into the changelog folder
    • name it $issue_id.$type for example (588.bug)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of removal, feature, bugfix, vendor, doc or trivial
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."
  • Target: for bugfix, vendor, doc or trivial fixes, target master; for removals or features target features;
  • Make sure to include reasonable tests for your change if necessary

Unless your change is a trivial or a documentation fix (e.g., a typo or reword of a small section) please:

  • Add yourself to AUTHORS;
andras-tim added 30 commits Jul 16, 2017
W191 indentation contains tabs
W292 no newline at end of file
W293 blank line contains whitespace
W391 blank line at end of file
indentation contains mixed spaces and tabs
indentation is not a multiple of four
unexpected indentation
continuation line under-indented for hanging indent
continuation line missing indentation or outdented
closing bracket does not match indentation of opening bracket’s line
closing bracket does not match visual indentation
continuation line with same indent as next logical line
continuation line over-indented for hanging indent
continuation line over-indented for visual indent
continuation line under-indented for visual indent
visually indented line with same indent as next logical line
continuation line unaligned for hanging indent
whitespace after ‘(‘
whitespace before ‘)’
whitespace before ‘:’
multiple spaces before operator
multiple spaces after operator
missing whitespace around operator
missing whitespace around arithmetic operator
missing whitespace after ‘,’, ‘;’, or ‘:’
multiple spaces after ‘,’
unexpected spaces around keyword / parameter equals
at least two spaces before inline comment
inline comment should start with ‘# ‘
block comment should start with ‘# ‘
multiple spaces after keyword
multiple spaces before keyword
andras-tim added 10 commits Jul 16, 2017
too many blank lines (3)
multiple imports on one line
module level import not at top of file
line too long (> 120 characters)
multiple statements on one line (colon)
multiple statements on one line (semicolon)
multiple statements on one line (def)
comparison to True should be ‘if cond is True:’ or ‘if cond:’
do not assign a lambda expression, use a def
@coveralls
Copy link

@coveralls coveralls commented Jul 17, 2017

Coverage Status

Coverage decreased (-0.01%) to 92.116% when pulling b2a5ec3 on andras-tim:fix-flake8-issues into 3578f4e on pytest-dev:master.

Loading

@andras-tim
Copy link
Contributor Author

@andras-tim andras-tim commented Jul 17, 2017

You can add the EP2017 sprint tag to this pull request too 😉

Loading

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Jul 18, 2017

I like this PR as this fixes the long standing issue of pep8 compatibility.

There's some points to consider though:

  1. This will change history a lot; I dislike this in general but for pytest I think this will be a win in the long term;
  2. This will cause some short term pain as we merge PRs which are not pep8 compatible because we were still ignoring some warnings at the time they were written; no big deal, should be short lived pain.
  3. This will cause some conflicts when next we have to merge with features; no big deal, I've already made a local merge and fixed the conflicts and updated the features only code using autopep8.

So in the end I think this should be merged, just wanted to clarify all pain points so everyone is on the same page. 😉

After this is merged we probably need some docs and a tox env that runs autopep8 with the correct parameters to simplify fixing pep8 errors for contributors. For reference I used:

autopep8 --in-place -r --max-line-length=120 --exclude vendored_packages _pytest testing

Loading

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Jul 18, 2017

Question: should we squash this before merging?

Loading

@RonnyPfannschmidt
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt commented Jul 18, 2017

i like the full history in this case

Loading

@andras-tim
Copy link
Contributor Author

@andras-tim andras-tim commented Jul 18, 2017

@nicoddemus feel free you squashing or not.

But one thing you should know, the autopep8 can't fix all pep8 rules properly and the flake8 has more rules than pep8.

Thx guys the fast reply.

Loading

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Jul 18, 2017

i like the full history in this case

I don't see the value in this case but that is fine.

Shall we merge this then?

Loading

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Jul 18, 2017

But one thing you should know, the autopep8 can't fix all pep8 rules properly and the flake8 has more rules than pep8

Thanks, noted. Do you have a suggestion for a better suited tool?

Loading

@andras-tim
Copy link
Contributor Author

@andras-tim andras-tim commented Jul 19, 2017

Thanks, noted. Do you have a suggestion for a better suited tool?

Unfortunately no, I don't.

Loading

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Jul 19, 2017

Unfortunately no, I don't.

I see, thanks.

If nobody opposes this, I plan to merge this later tonight and update the features branch accordingly. I will also add a tox environment that runs autopep8 on the code with the correct options, and update the development guide instructing to use it.

Loading

@andras-tim
Copy link
Contributor Author

@andras-tim andras-tim commented Jul 19, 2017

@nicoddemus, what's your exact plan w/ autopep8 in tox? I think this can be useful when a user wants to fix his coding problems, but I do not prefer to run by CI. (Who will be the owner? Will it be squashed or amended? What will be, when autopep8 can't fix all problem?)

So I prefer to use autopep8 as a one-time tool, what anyone can use when he gets new errors, and too lazy to fix by hand.

Loading

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Jul 19, 2017

what's your exact plan w/ autopep8 in tox?

My idea is to just use tox like a convenience around a virtual environment with known dependencies. In that case tox just facilitates creating a virtual environment with the required dependencies and executing some commands. We already have tox envs that act like this: regen updates the local .rst files with local pytest commands, docs generates the documentation, etc. This type of usage is becoming common in the tox community, see tox-dev/tox#427.

Like regen, this new tox environment is just a local development tool, never to be run on CI.

Loading

@andras-tim
Copy link
Contributor Author

@andras-tim andras-tim commented Jul 19, 2017

Like regen, this new tox environment is just a local development tool, never to be run on CI.

ok 👍

Loading

@nicoddemus nicoddemus merged commit b2a5ec3 into pytest-dev:master Jul 19, 2017
2 checks passed
Loading
@andras-tim andras-tim deleted the fix-flake8-issues branch Jul 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants