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

[RFC] Add qa env #1180

Merged
merged 1 commit into from Jan 1, 2020
Merged

[RFC] Add qa env #1180

merged 1 commit into from Jan 1, 2020

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Jul 15, 2018

Ignores tests with flake8 completely for now.

Many failures, so should not really get enabled yet, but might be a good base for local checking.

@codecov-io
Copy link

codecov-io commented Jul 16, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1180   +/-   ##
=======================================
  Coverage   92.86%   92.86%           
=======================================
  Files          57       57           
  Lines        6910     6910           
=======================================
  Hits         6417     6417           
  Misses        493      493

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 d6306a0...3c2994e. Read the comment docs.

@davidhalter
Copy link
Owner

I generally like it, if we enable it, we should probably also enable it on travis.

I already have flake8 running and ignore the warning about spacing around operators (because that's just not PEP8). Generally I would probably advice creating a configuration that most of Jedi works with. We could then start arguing which rules should be followed in the future and which ones not. I generally think that the PEP8 part of flake8 is pretty poorly done and could be improved tremendously. I even tried to do that in parso, but I'm still not sure how to do certain indentation rules. So I just never really made that public.

@blueyed
Copy link
Contributor Author

blueyed commented Jul 17, 2018

Sounds good.

What do you think about https://github.com/ambv/black/?
E.g. the pytest projects has adopted it, and has run just all of its code through it.

@davidhalter
Copy link
Owner

Maybe later. At the moment I'm not interested in tools re-formatting the whole code base. I think it's actually in a good shape and I also think that style guides are not that important - at least not all the details. I care much more about good naming, consistency and code readability in general.

@blueyed
Copy link
Contributor Author

blueyed commented Jul 18, 2018

Sure.

My issue here is using Neomake and its flake8 maker everywhere, and often that results in warnings that are not that important, but still being reported.
In the end for jedi this means to get the flake8 config in sync with what is being used.

@davidhalter
Copy link
Owner

Yeah, I can understand that and I think we should actually properly configure it in setup.cfg, so that at least the warnings are gone :).

@blueyed
Copy link
Contributor Author

blueyed commented Aug 14, 2018

Do you prefer "line break before binary operator", or after? (W503/W504)

I prefer the latter, i.e. we would ignore W503 then.

@davidhalter
Copy link
Owner

davidhalter commented Aug 15, 2018

Can you quickly look at the git commits on the typeshed branch? I unfortunately commited the changes to ../setup.cfg there. W503 is already ignored there.

BTW: This change was thanks to you, I didn't really know that it was that easy to specify that or I would have done it a while ago.

Ignores tests with flake8 completely for now.
@davidhalter
Copy link
Owner

Merged and seems to work. I'm now ignoring F401 in tox.ini, because I still want that warning in general, but just not for the linter. I feel like this is really a flake8 problem, because the linter is just very simple. This is my workaround.

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

3 participants