Skip to content

black codestyle #3239

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

Closed
wants to merge 1 commit into from
Closed

black codestyle #3239

wants to merge 1 commit into from

Conversation

ferrine
Copy link
Member

@ferrine ferrine commented Oct 27, 2018

With this tool code looks much prettier. This is the first preview commit, we should integrate this with CI if it is all good

@ferrine
Copy link
Member Author

ferrine commented Oct 27, 2018

I have concerns about that as some PRs are in progress and will be affected by changes.

@twiecki
Copy link
Member

twiecki commented Oct 27, 2018

I agree that this looks much better. Seemed like you used an older commit, however.

I think this could be a good time for this, I don't think there are many active WIP commits, mainly #3214

@ferrine
Copy link
Member Author

ferrine commented Oct 27, 2018

Yes, things changed in a week and I did not git pull. Any more opinions?

@ferrine
Copy link
Member Author

ferrine commented Oct 27, 2018

I got issues with rebase and just recreated the branch, I suppose same issues will be with ongoing PRs

@ferrine
Copy link
Member Author

ferrine commented Oct 27, 2018

We can wait until #3214 and then do the refactoring not to make a mess. Another option is trying to rebase it on top of this branch to check if it can be done with little pain

@canyon289
Copy link
Member

canyon289 commented Oct 27, 2018

Copy paste from slack

Black is used in ArviZ

Its mostly good, but when its bad, I think its pretty bad
it is consistent though

@springcoil
Copy link
Contributor

How does one use this?

@fonnesbeck
Copy link
Member

It does look better on the whole. I don't think we need to enforce this every time someone commits code, but perhaps run it prior to releases?

@ColCarroll
Copy link
Member

It is pretty quick as a first check on CI: we run

python -m black -l 100 --check arviz/

which takes 0.1s on my machine. Running on PyMC3, it takes 7s, but it is finding actual errors. To change the code, you leave out the --check flag.

I find the biggest benefit for a project is fewer nitpicking formatting requests - within seconds of making a PR, you're reminded to run black on your own code and resubmit. The biggest benefit for me is that most editors (vim or VS code) will run it automatically on save, and I'm not even thinking about line length or making shorter variable names.

Note that it does not replace pylint or pydocstyle, but it conforms to both.

@ferrine
Copy link
Member Author

ferrine commented Oct 27, 2018

As I remember pycodestyle is less restrictive and thus less consistent

Oh, misread, pydocstyle?

@canyon289
Copy link
Member

canyon289 commented Oct 27, 2018

It is pretty quick as a first check on CI: we run

python -m black -l 100 --check arviz/

Note that it does not replace pylint or pydocstyle, but it conforms to both.

You can also just run black --check arviz/ and it'll automatically pick up 100 line length from a settings file, and black has a bash entry point. Exactly the same as above, just save some keystrokes

@canyon289
Copy link
Member

As I remember pycodestyle is less restrictive and thus less consistent

Oh, misread, pydocstyle?

pydocstyle checks to make sure docs are in correct format which black doesn't do.
pylint checks for unused imports, or variables, which is useful as well.

All three work well together!

@aseyboldt
Copy link
Member

aseyboldt commented Oct 28, 2018

I don't think I really like this idea. This touches most of the code in pymc3, so a simple git blame will never show much useful information after we merge this. I agree that the formatting looks nice (mostly, sometimes I prefer the original), but in my opinion it really isn't worth it. If we write new code, by all means use black, same if we mostly rewrite a file. But touching everything just to make it look a bit more pretty seems a bit extreme to me.

@twiecki
Copy link
Member

twiecki commented Oct 31, 2018

As always, @aseyboldt makes a pretty compelling case and from the likes on his comment it seems like there is critical mass in agreement, so I'm closing this.

@twiecki twiecki closed this Oct 31, 2018
@ferrine
Copy link
Member Author

ferrine commented Oct 31, 2018

Yes, that was a very fair point

@eigenfoo eigenfoo deleted the black-codestyle branch January 1, 2020 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants