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

Add pre-commit configuration (Black) #35

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

evelyn9191
Copy link
Contributor

No description provided.

@evelyn9191
Copy link
Contributor Author

The first part of the pre-commit PR. @rkdarst

@rkdarst
Copy link
Member

rkdarst commented Mar 22, 2020

So, about this... while these days I see some benefit of strict code formatting (I didn't before, and still wouldn't necessarily use it for my projects), after thinking more I don't feel fully comfortable taking it into use in this project if I'm basically watching it for someone else, who may start working on it later this year.

What do you think? I hope this is OK. Any other improvements I would like, and we should try to get a release out...

@evelyn9191
Copy link
Contributor Author

I think that when more people contribute to the code (which, given that this is an open-source library, we can expect people to do), it is very helpful to keep it clean, readable and reusable.

I can understand the reasons against more strict formatters that won't let you pass until you fix the issue, such as mypy or flake8, I don't see Black as an obstruction - it is an automatic formatting tool. Once you run pre-commit install, you don't really have to do anything else - when set up correctly, it will format your code before you commit and you don't need to do anything extra. The point of Black is to let you write the code as you want, without worrying about any formatting because it does it for you automatically. So whereas I get the reasons for not accepting the above mentioned formatters, I don't quite follow the reasons behind not accepting this PR.

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.

2 participants