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

tox: checkqa: check isort via flake8-isort #420

Merged
merged 1 commit into from Nov 28, 2017

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Oct 17, 2017

No description provided.

@blueyed
Copy link
Contributor Author

blueyed commented Oct 17, 2017

Failure can be seen in https://circleci.com/gh/pinax/pinax-stripe/2217.

@codecov
Copy link

codecov bot commented Oct 17, 2017

Codecov Report

Merging #420 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #420   +/-   ##
=======================================
  Coverage   99.39%   99.39%           
=======================================
  Files          34       34           
  Lines        1651     1651           
  Branches      134      134           
=======================================
  Hits         1641     1641           
  Misses          5        5           
  Partials        5        5
Flag Coverage Δ
#py27dj110 99.39% <ø> (ø) ⬆️
#py27dj111 99.39% <ø> (ø) ⬆️
#py27dj18 99.39% <ø> (ø) ⬆️
#py33dj18 99.39% <ø> (ø) ⬆️
#py34dj110 99.39% <ø> (ø) ⬆️
#py34dj111 99.39% <ø> (ø) ⬆️
#py34dj18 99.39% <ø> (ø) ⬆️
#py35dj110 99.39% <ø> (ø) ⬆️
#py35dj111 99.39% <ø> (ø) ⬆️
#py35dj18 99.39% <ø> (ø) ⬆️
#py35djmaster 99.39% <ø> (ø) ⬆️
#py36dj111 99.39% <ø> (ø) ⬆️
#py36djmaster 99.39% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pinax/stripe/forms.py 98.07% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fda68d4...f1406db. Read the comment docs.

@codecov
Copy link

codecov bot commented Oct 17, 2017

Codecov Report

Merging #420 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #420   +/-   ##
=======================================
  Coverage   99.39%   99.39%           
=======================================
  Files          34       34           
  Lines        1829     1829           
  Branches      166      166           
=======================================
  Hits         1818     1818           
  Misses          5        5           
  Partials        6        6
Flag Coverage Δ
#py27dj110 99.07% <ø> (ø) ⬆️
#py27dj111 99.07% <ø> (ø) ⬆️
#py27dj18 99.34% <ø> (ø) ⬆️
#py34dj110 99.07% <ø> (ø) ⬆️
#py34dj111 99.07% <ø> (ø) ⬆️
#py34dj18 99.34% <ø> (ø) ⬆️
#py34dj20 99.07% <ø> (ø) ⬆️
#py35dj110 99.07% <ø> (ø) ⬆️
#py35dj111 99.07% <ø> (ø) ⬆️
#py35dj18 99.34% <ø> (ø) ⬆️
#py35dj20 99.07% <ø> (ø) ⬆️
#py36dj111 99.07% <ø> (ø) ⬆️
#py36dj20 99.07% <ø> (ø) ⬆️
#py36dj20psql 99.07% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 112111f...199e20d. Read the comment docs.

@blueyed
Copy link
Contributor Author

blueyed commented Oct 17, 2017

@paltman
this is crucial to get PRs green (especially with checkqa being required before tests).

@paltman
Copy link
Member

paltman commented Oct 20, 2017

A big reason I had avoided flake8-isort was I preferred to keep all the CI config in one place in tox.ini and use the setup.cfg for packaging config like the universal wheel attribute.

Not following why it's critical to use this wrapper tool.

@blueyed
Copy link
Contributor Author

blueyed commented Oct 20, 2017

It integrates nicely with running flake8 yourself locally, e.g. with Neomake in your editor.

@blueyed
Copy link
Contributor Author

blueyed commented Oct 23, 2017

I suggest also moving the flake8 config to setup.cfg.

@paltman
Copy link
Member

paltman commented Oct 23, 2017

I would prefer to do the opposite and move things into tox.ini and leave setup.cfg for packaging.

@paltman
Copy link
Member

paltman commented Oct 23, 2017

what's the argument for moving things out of tox.ini?

@blueyed
Copy link
Contributor Author

blueyed commented Oct 23, 2017

The main argument seems to be that flake8 does not look at tox.ini by default.

I am constantly running into issues where lint complains about single quotes etc (and I get no feedback for the actual tests (see #418)) because of that.
I use Neomake to run flake8, but that does not help.

IMHO setup.cfg is a good place for config/cfg, while tox.ini is very much specific to tox.

@paltman
Copy link
Member

paltman commented Oct 23, 2017 via email

@blueyed
Copy link
Contributor Author

blueyed commented Oct 23, 2017

Ok, then it might be just flake8-isort.
But anyway, setup.cfg feels like config (look at the .cfg and setup there).
And you need it for the wheels config anyway.

Feel free to close this if you feel inclinded, but I think having a more config-like central place for configuration is a good trade-off for making flake8-isort work out of the box.

@blueyed
Copy link
Contributor Author

blueyed commented Oct 23, 2017

And besides that, config/isort is one of the less important issues of this project currently.

It's just because of open (and addressed in PR) issues like #418 that config is important for me to fix - since it is currently very disruptive.

@blueyed
Copy link
Contributor Author

blueyed commented Oct 23, 2017

Even here we still see that checks for py{35,36}master are waiting for status to be reported, but I've asked several times already to just remove them from the protected branch.
This would make like 20+ PRs green already, which are currently orange (looking like CI would have hiccups). But it still takes an extra step to not require PRs for being up to date with master (#422).

@paltman
Copy link
Member

paltman commented Oct 23, 2017

Fixed the master checking

@blueyed
Copy link
Contributor Author

blueyed commented Oct 23, 2017

PRs still have to be up-to-date with master, no?

@blueyed blueyed closed this Nov 6, 2017
@blueyed blueyed deleted the checkqa-isort-via-flake8 branch November 6, 2017 19:36
@blueyed blueyed mentioned this pull request Nov 6, 2017
@blueyed blueyed restored the checkqa-isort-via-flake8 branch November 8, 2017 17:51
@blueyed blueyed reopened this Nov 8, 2017
@blueyed
Copy link
Contributor Author

blueyed commented Nov 8, 2017

@paltman
Would you accept this after all?
Then I would rebase it / fix the conflicts.

@paltman
Copy link
Member

paltman commented Nov 8, 2017

I thought flake8-isort didn't read from tox.ini (thus the need for this PR gforcada/flake8-isort#35

@blueyed
Copy link
Contributor Author

blueyed commented Nov 8, 2017

This change does not seem to be in flake8-isort yet.
But even if it was in, I still consider setup.cfg more the source of tool config (and you need the file anyway).

@blueyed
Copy link
Contributor Author

blueyed commented Nov 27, 2017

@paltman
You have not answered my previous question about taking it when I would fix the conflicts.
Please do so, and close it in case you really want to keep generic config in tox.ini.

@paltman
Copy link
Member

paltman commented Nov 28, 2017

I'm fine with it now. Can you resolve the conflicts and I'll get it merged. I appreciate your patience with me.

This moves the isort cfg to setup.cfg.
@paltman paltman merged commit d0f2d69 into pinax:master Nov 28, 2017
@blueyed blueyed deleted the checkqa-isort-via-flake8 branch November 28, 2017 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants