-
Notifications
You must be signed in to change notification settings - Fork 17
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
Use code style Black #22
Use code style Black #22
Conversation
@dguest I'm debuging some regex with the |
e5e8b07
to
2b4f826
Compare
@dguest this is ready for review now. |
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.
Looks good overall. I don't have a problem with consistent formatting. There was just one place where I'm a bit surprised by black's choice (putting a space after the first index in a slice, i.e. list[x:]
-> list[x :]
.
def short_fmt(fmt): | ||
removed_pfx = ['DAOD_'] | ||
for pfx in removed_pfx: | ||
if fmt.startswith(pfx): | ||
return fmt[len(pfx):] | ||
return fmt[len(pfx) :] |
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.
this one seems a bit weird to me: does black really insist on a space after each argument in a slice?
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 agree that looks a bit weird, but I think this is a PEP8 adherence thing: psf/black#157
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.
will black allow a space on both sides of the len(pfx)
?
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.
No. If you paste the following snippet
def short_fmt(fmt):
removed_pfx = ['DAOD_']
for pfx in removed_pfx:
if fmt.startswith(pfx):
return fmt[ len(pfx): ]
into the playground (https://black.now.sh/) you get back
def short_fmt(fmt):
removed_pfx = ["DAOD_"]
for pfx in removed_pfx:
if fmt.startswith(pfx):
return fmt[len(pfx) :]
datasets = [ | ||
ds | ||
for ds in datasets | ||
if int(ds['reqid']) in range(int(r[0]), int(r[1])) | ||
] |
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.
This one is also a bit awkward, but I guess this is Black's way of telling me that I should just write a for
loop.
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, to be clear, Black will not always make your code look beautiful. In many cases it will make it a bit less so. The major benefit Black gives is total offloading of mental effort of formatting, and making every commit contain only the core essence of the change (not developer style). There is a trade off here, and it might not be the right one for each project.
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'm fine with it. This is too long a list comprehension anyway.
@@ -0,0 +1,12 @@ | |||
[tool.black] | |||
line-length = 79 | |||
target-version = ['py36', 'py37', 'py38'] |
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 just realized that this is a bit weird: for better or worse (mostly worse) the target version is py27.
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.
Black doesn't support or work with Python 2 at all. So target-version
is a list of just the Python 3 versions it is targeting.
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.
To be clear, that doesn't mean that Python 2 support of the code needs to be dropped. Just that to actually run Black you need a Python 3 runtime.
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.
Interesting, I guess as long as black doesn't have a tendency to make changes that are more than aesthetic in python 2 this is fine.
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.
Black does introduce trailing commas in some tuples or lists, which will be viewed as invalid syntax in Python 2. However, this is not the case for pandamonium
, and so it is unaffected.
FWIW, I have never had this break code for me.
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 don't even think that's a syntax error in python 2:
> python2 -c 'l=[1,2,3,]; t=(3,4,5,); print(l,t)'
([1, 2, 3], (3, 4, 5))
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.
Huh. Yeah, I guess I'm wrong on where this causes problems. I have seen examples of Black breaking working Python 2 code, but apparently I don't remember how and this is hard to do.
@dguest Ping on this given a recent update to Black. |
Thanks @dguest |
All you can handle bro! |
Go
Adds a
pyproject.toml
with Black config options, and then applies them to all thepandamonium
files. Additionally, add a.pre-commit-config.yaml
for a Blackpre-commit
hook. Additionally add the Code style Black badge to theREADME
.