-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
STY: use black #26926
Comments
+1. Not having to worry about formatting is nice. We use it for dask-ml and
the reception has been positive.
Dask is adopting it soon.
The tricky part is of course the outstanding PRs. It may be worth doing a
mini-sprint to get as many of those resolved (merged or closed) as possible
before applying black.
For the remainder, perhaps a maintainer could push a commit applying black
on the PR and fixing merge conflicts, so that the contributor doesn't need
to.
I would also recommend pairing this with a pre-commit-config in the repo:
#23616
…On Tue, Jun 18, 2019 at 9:41 AM Jeff Reback ***@***.***> wrote:
I have seen a number of other project adopt:
https://pypi.org/project/black/, as well as use in my company.
This basically is a style formatter that plays nice with flake8 and is
completely opionated, the only option is line-length. This is a good thing.
This would involve a one-time updating of code (though we can do it
somewhat incrementally).
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#26926?email_source=notifications&email_token=AAKAOIS4D26NSFOGN4EDS6TP3DX3JA5CNFSM4HZAZSP2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4G2FIIAQ>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKAOISJFFSGEOSKMIMNLODP3DX3JANCNFSM4HZAZSPQ>
.
|
Could we start with requiring it for new code? (assuming black has a reformat-diff-only option) |
+1 in principle just tried it with VSCode extension. can't apply to a selection, not a problem if code is already blackened. raise ValueError('cannot convert float '
'NaN to integer') raise ValueError(
"cannot convert float " "NaN to integer"
) may want a second pass to tidy things like this. from pandas.core.dtypes.generic import (
ABCDataFrame, ABCDateOffset, ABCDatetimeArray, ABCIndexClass,
ABCMultiIndex, ABCPandasArray, ABCPeriodIndex, ABCSeries,
ABCTimedeltaArray, ABCTimedeltaIndex) from pandas.core.dtypes.generic import (
ABCDataFrame,
ABCDateOffset,
ABCDatetimeArray,
ABCIndexClass,
ABCMultiIndex,
ABCPandasArray,
ABCPeriodIndex,
ABCSeries,
ABCTimedeltaArray,
ABCTimedeltaIndex,
) may need a test to make sure it plays nice with isort |
With a bit of configuration, it plays fine with isort.
Black only supports pyproject.toml for configuration, which we've had to
remove for now, but I think it's safe to re-introduce.
If we do this, we should revisit the 80-character line length limit. I
think now that we're adding type annotations something a bit longer makes
sense (maybe black's default of 88).
…On Tue, Jun 18, 2019 at 2:01 PM Simon Hawkins ***@***.***> wrote:
+1 in principle
just tried it with VSCode extension.
can't apply to a selection, not a problem if code is already blackened.
raise ValueError('cannot convert float '
'NaN to integer')
raise ValueError(
"cannot convert float " "NaN to integer"
)
may want a second pass to tidy things like this.
from pandas.core.dtypes.generic import (
ABCDataFrame, ABCDateOffset, ABCDatetimeArray, ABCIndexClass,
ABCMultiIndex, ABCPandasArray, ABCPeriodIndex, ABCSeries,
ABCTimedeltaArray, ABCTimedeltaIndex)
from pandas.core.dtypes.generic import (
ABCDataFrame,
ABCDateOffset,
ABCDatetimeArray,
ABCIndexClass,
ABCMultiIndex,
ABCPandasArray,
ABCPeriodIndex,
ABCSeries,
ABCTimedeltaArray,
ABCTimedeltaIndex,
)
may need a test to make sure it plays nice with isort
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#26926?email_source=notifications&email_token=AAKAOIXPAMKTZXYNVFKUWF3P3EWIVA5CNFSM4HZAZSP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX7UKFA#issuecomment-503268628>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKAOIS476VFPZT3JN3R6M3P3EWIVANCNFSM4HZAZSPQ>
.
|
If it helps in any way, here is an isort config which plays fine with black and pre-commit in my projects
|
So we discussed it last week on the call, and no strong objections were raised (and indeed, it does deliberately not support only blackening the diff to do it incrementally). So we will try to push this during the sprints now. |
i'd also be happier with not increasing the line length. the standard 79 chars would be good. for accessibility I prefer to have larger fonts, so regardless of the laptop resolution, the shorter line length helps with side by side files. |
I personally like the somewhat longer line length, but certainly understand those arguments to keep it at 79 (I also regularly have files side by side, and it gets more tight in that way). With the tendency of black to create more horizontal lines, I think it is nice to use their default of 88. And as Tom said above with adding more typing annotations, a longer line length can also help there. There is one practical problem with changing their default. Black uses a |
Although not the recommended approach, would using the pre-commit hook skirt this issue? https://black.readthedocs.io/en/stable/version_control_integration.html#version-control-integration |
Closable? |
I have seen a number of other project adopt: https://pypi.org/project/black/, as well as use in my company.
This basically is a style formatter that plays nice with flake8 and is completely opionated, the only option is line-length. This is a good thing.
This would involve a one-time updating of code (though we can do it somewhat incrementally).
The text was updated successfully, but these errors were encountered: