-
-
Notifications
You must be signed in to change notification settings - Fork 422
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
Black formatter PR #282
Black formatter PR #282
Conversation
Hello @AhmetCanSolak! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2019-05-23 20:16:00 UTC |
napari/components/_viewer/model.py
Outdated
@@ -31,16 +31,19 @@ class Viewer: | |||
These key bindings are executed instead of any layer specific key | |||
bindings. | |||
""" | |||
def __init__(self, title='napari'): | |||
|
|||
def __init__(self, title="napari"): |
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.
Is the any way to configure black to use '
instead of "
?
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 as far as i know. This is something black
has a strong preference. To me also it makes sense, as double "
is used for strings as a widely popular convention. On top of that, it is consistent with C/OpenCL and so. IMO we are very likely to depend such languages/tools in the long run.
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.
yeah - i just found a super long hacker news post on this one! Let's just say some very strong opinions on either side
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.
Everybody(including me) has an opinion on everything(especially in python world) :D that's why maybe better to decide according to priorities instead of opinions.
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.
btw, good news it is possible to stop black
on this with: --skip-string-normalization
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 - very interesting, I just read this thread here psf/black#118, and the main developer very reasonably added that skip, which can be achieved with the short-cut black . -S
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 pretty happy with how black
has done the new line spacing, and I love how it has inserted white space between operators, I think that will cut down the review time on my PRs quite a bit!
One thing I'd be curious to see though is the output of running with the -S
(--skip-string-normalization
) command. I prefer the ability to mix '
and "
, using '
for things like dictionary names as I find them easier to read. If we like the results then I'd vote for black -S
Yeah I gotta say I'm pretty happy with the result! Thanks for running this experiment, @AhmetCanSolak!
It'd be awesome if black could also format numpy-style docstrings, but that's a bit much to ask. =) I'm moving my position from 👎 to neutral. =) CC @stefanv (as we have considered |
Thank you @sofroniewn and @jni ! Glad you guys like it and yes having |
all |
I'm fine with one PR that does all the reformatting too and add the pre-commit. It looks like the build is failing now as it is not finding the right version of |
eerh pypi and conda versions are different :( fixing 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'd prefer to see the pre-commit stuff as a separate PR split into three commits:
- actual pre-commit code/setup
- documentation (README banner/CONTRIBUTING section)
- the results of
pre-commit run --all-files
We could also make this PR just the results of black -S
and the pre-commit code as a separate PR.
Co-Authored-By: Kira Evans <[email protected]>
…code as discussion ended
Updated this PR. Will be happy if you guys @jni @sofroniewn @kne42 can review so i can continue with formatting the entire repo with black. |
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.
@AhmetCanSolak I think @sofroniewn's comment is worth addressing in this PR, as well as line length. I think both should be quick work!
CONTRIBUTING.md
Outdated
|
||
With help of pre-commit, your future commits will be reformatted with our black configuration | ||
which includes settings like `skip-string-normalization = true` and `max-line-length = 79`. | ||
If you like to learn more please check [black](https://github.com/python/black). |
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.
Can you change it to "If you would like to learn more", slightly better grammatically. Otherwise looks ready to merge
Boom! =) |
Description
This PR is to see how
black
formatter would change some of our code.Please check the details and let each other know about your comments.
Type of change
References
https://github.com/python/black
How has this been tested?
Will be tested on review manually.
Final checklist: