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

MNT Adds black config and fixes formatting before black #19031

Merged
merged 4 commits into from
Jun 12, 2021

Conversation

thomasjpfan
Copy link
Member

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.

@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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will get conflict with this: #19005
Do you think that we could merge #19005 first :)

Copy link
Member Author

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.

Copy link
Contributor

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
Copy link
Member

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?

Copy link
Member Author

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)

Base automatically changed from master to main January 22, 2021 10:53
Copy link
Member

@rth rth left a 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?

setup.cfg Outdated Show resolved Hide resolved
Copy link
Member

@lorentzenchr lorentzenchr left a 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
Copy link
Member

@lorentzenchr lorentzenchr Jun 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor

@chkoar chkoar Jun 14, 2021

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>
@rth rth merged commit 9a13bdf into scikit-learn:main Jun 12, 2021
@thomasjpfan
Copy link
Member Author

Thank you for pushing this through @rth and @lorentzenchr !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants