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

Format using black #156

Closed
ianhi opened this issue Oct 6, 2020 · 6 comments · Fixed by #171
Closed

Format using black #156

ianhi opened this issue Oct 6, 2020 · 6 comments · Fixed by #171
Labels
enhancement New feature or request

Comments

@ianhi
Copy link
Contributor

ianhi commented Oct 6, 2020

Figure I'm on a roll of suggesting cosmetic rather than substantive changes so may as well continue.

Problem

As more people contribute to this repo it will become more and more difficult to maintain a consistent code style and keep everything readable.

Proposed Solution

Enforce all code to be autoformatted by https://github.com/psf/black. This is nice because unlike tools like pep8 or flake8 etc it will actually modify the code to conform to the style guide for you.

Additional context

One of the few configurable options is the line length, so this woud not constrain line length.

I got hooked on this when we added it to jupyterlab-git and now I use it in all my personal projects. Here is an example PR of formatting everything + adding a formatting check to tests + adding a precommit git hook to autoformat jupyterlab/jupyterlab-git#763

It also is supported in editors and there is even an extension integrating it into jupyterlab https://jupyterlab-code-formatter.readthedocs.io/en/latest/index.html

@ianhi ianhi added the enhancement New feature or request label Oct 6, 2020
@henrypinkard
Copy link
Member

I've never used jupyterlab, so I don't really understand everything you're suggesting here. Does it work with Pycharm? Are there additional setup/configuration things that would be needed to do it? Can it be applied to notebooks that people PR as well?

I think an autoformatter is great in principle, though I'm hesitant to implement anything that will add extra steps for novice python users to make contributions. I have had the experience before of contributing to an open source project and having to figure out how to use their formatting scripts to do so, which I found quite annoying.

Especially considering the relatively small size of the Python codebase in this project, I would want to make sure this doesn't introduce any barrier of entry to contributing

@ianhi
Copy link
Contributor Author

ianhi commented Oct 14, 2020

I've never used jupyterlab, so I don't really understand everything you're suggesting here. Does it work with Pycharm?

Ah, my bad. Jupyterlab was a bit of red herring, I mentioned it because it's surprising that it works for notebooks because black is designed to work on files. black is first and foremost a command line tool, you can run it python -m black . (or black .) from the command prompt and it will correctly format all the files in that directory. There are also lots of integrations into IDEs e.g. for pycharm: https://black.readthedocs.io/en/stable/editor_integration.html#pycharm-intellij-idea

Can it be applied to notebooks that people PR as well?

Yes, though this is maybe something that would be better to do manually. I use https://jupyterlab-code-formatter.readthedocs.io/en/latest/index.html and there's an option to format the entire notebook

I think an autoformatter is great in principle, though I'm hesitant to implement anything that will add extra steps for novice python users to make contributions. I have had the experience before of contributing to an open source project and having to figure out how to use their formatting scripts to do so, which I found quite annoying.

I would want to make sure this doesn't introduce any barrier of entry to contributing

I've been there, the flake8 linting errors when contributing to matplotlib can be extremely confusing. This can definitely be implemented in such a way that it doesn't introduce a barrier. Essentially the direction would be:

  1. make your PR and get the content reviewed
  2. at the end of PR ask them to do:
    • pip install black
    • black . # from command line

this can also be set up so that it runs every time you git commit but thats not necessary.

This could also be something that just gets done every now and again as a one-off

@henrypinkard
Copy link
Member

Got it. Yeah that sounds like a good idea then. Certainly at minimum as something that can be done manually from time to time, and then on top of that if there's some mechanism to automatically suggest to contributors to do it when PRing. Would also be good for the project's swag to have one of those Code style: black

@ianhi
Copy link
Contributor Author

ianhi commented Oct 14, 2020

Would also be good for the project's swag to have one of those

That's honestly the strongest possible argument for it :)

there's some mechanism to automatically suggest to contributors to do it when PRing

Using pre-commit will do this.

You can make the contributing install instructions look something like

git clone <your fork>
cd pycro-manager
pip install -e ".[dev]"
pre-commit install

and then everything will be set up.

@ianhi
Copy link
Contributor Author

ianhi commented Oct 14, 2020

And you can add an automatic check on to run on PRs using github actions. see here https://github.com/ianhi/mpl-interactions/blob/04eb663503dcd80556c2a5e7ac74348464af2621/.github/workflows/test.yml#L52

@henrypinkard
Copy link
Member

Sounds great, go for it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants