-
-
Notifications
You must be signed in to change notification settings - Fork 25.5k
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
MNT Adds black config and fixes formatting before black #19031
MNT Adds black config and fixes formatting before black #19031
Conversation
@deprecated("The attribute 'counts_' is deprecated in 0.24" # type: ignore | ||
" and will be removed in 0.26.") | ||
@deprecated( # type: ignore | ||
"The attribute 'counts_' is deprecated in 0.24" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, lets merged that first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#19005 is merged
"If you observe this warning while using RFE " | ||
"or SelectFromModel, use the importance_getter " | ||
"parameter instead.") | ||
@deprecated( # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's already a comment for mypy
errors just above. Can we remove one of them or do they address different things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove one of them. Although I do not mine the verbosity here.
Side note: I think it would be nice to have a _deprecated_property
that just does the correct thing regarding order + mypy. (If possible)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @thomasjpfan !
I resolved the conflicts. It would be nice to move forward with black.
Maybe @glemaitre or @lorentzenchr could have a look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
setup.cfg
Outdated
# Default flake8 3.5 ignored flags | ||
ignore=E121,E123,E126,E226,E24,E704,W503,W504 | ||
ignore=E121,E123,E126,E226,E24,E704,W503,W504,E203,E731,E741 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore=E121,E123,E126,E226,E24,E704,W503,W504,E203,E731,E741 | |
ignore= | |
E24, # check ignored by default in flake8. Meaning unclear. | |
E121, # continuation line under-indented | |
E123, # closing bracket does not match indentation | |
E126, # continuation line over-indented for hanging indent | |
E203, # space before : (needed for how black formats slicing) | |
E226, # missing whitespace around arithmetic operator | |
E704, # multiple statements on one line (def) | |
E731, # do not assign a lambda expression, use a def | |
E741, # do not use variables named ‘l’, ‘O’, or ‘I’ | |
W503, # line break before binary operator | |
W504 # line break after binary operator |
Pandas does that, too. See https://github.com/pandas-dev/pandas/blob/master/setup.cfg
What's the meaning of E24? I couldn't find its meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I also couldn't find what E24 means but it's part of the error codes ignored by default so we have no reason to enable it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll merge your suggestion and PR. I don't think it's worth blocking it to investigate what E24 does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the meaning of E24? I couldn't find its meaning.
I think that E24
refers to the E24x
family of errors.
E241: multiple spaces after separator
E242: tab after separator
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Thank you for pushing this through @rth and @lorentzenchr ! |
Reference Issues/PRs
Related to #18948
What does this implement/fix? Explain your changes.
The purpose of this PR is to make style changes such that #18948 can be a PR that just applies black.
Any other comments?
@rth The configuration settings you #18948 (review) are here as well.