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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add E231 ignore to flake8 checs, conflicts with black #2108

Closed
wants to merge 2 commits into from

Conversation

kumuji
Copy link
Contributor

@kumuji kumuji commented Jun 7, 2020

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Fixes problem with pep8speaks

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 馃檭

@mergify mergify bot requested a review from a team June 7, 2020 21:13
@Borda Borda added bug Something isn't working ci Continuous Integration labels Jun 7, 2020
@Borda Borda requested a review from awaelchli June 7, 2020 21:27
@mergify mergify bot requested a review from a team June 7, 2020 21:28
@codecov
Copy link

codecov bot commented Jun 7, 2020

Codecov Report

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

@@          Coverage Diff           @@
##           master   #2108   +/-   ##
======================================
  Coverage      86%     86%           
======================================
  Files          75      75           
  Lines        4710    4710           
======================================
  Hits         4067    4067           
  Misses        643     643           

@mergify mergify bot requested a review from a team June 8, 2020 06:48
@awaelchli
Copy link
Member

I don't understand this change. If one does not use black, this ignores the whitespace rules. Pretty sure this is undesired? With my comment in the other PR I did not mean to say we should ignore the rule simply because I saw a warning. It is likely a bug with black itself.

@Borda Borda self-requested a review June 8, 2020 07:45
@Borda
Copy link
Member

Borda commented Jun 8, 2020

just thinking what about dictionary {'any-key': 123}

@kumuji mind check is something similar was reported in Black already - that PEP speaks complain about black patterns?

@kumuji
Copy link
Contributor Author

kumuji commented Jun 11, 2020

just thinking what about dictionary {'any-key': 123}

@kumuji mind check is something similar was reported in Black already - that PEP speaks complain about black patterns?

I haven't seen anything like it in black repo, not sure if it is a bug though. It just appears not frequently.

@kumuji kumuji requested review from justusschock and removed request for a team June 13, 2020 13:16
@mergify mergify bot requested a review from a team June 13, 2020 13:17
@Borda
Copy link
Member

Borda commented Jun 23, 2020

hey, any update here? 馃

@kumuji
Copy link
Contributor Author

kumuji commented Jun 24, 2020

hey, any update here? raccoon

Seems like this error rarely appears, probably it is not necessary.

@Borda
Copy link
Member

Borda commented Jun 24, 2020

just looking around and found psf/black#1289 and psf/black#429
and according to PEP8 black is wrong with this particular formating... https://www.python.org/dev/peps/pep-0008/#pet-peeves
Let's stick with PEP8 and if/when black deals with it, we'll upgrade it... 馃惏

@Borda Borda closed this Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ci Continuous Integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants