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

[ENHANCEMENT] black formatting #448

Closed
dbrakenhoff opened this issue Jan 9, 2023 · 7 comments
Closed

[ENHANCEMENT] black formatting #448

dbrakenhoff opened this issue Jan 9, 2023 · 7 comments
Assignees
Labels
enhancement Indicates improvement of existing features

Comments

@dbrakenhoff
Copy link
Member

As proposed by @martinvonk in #389, use black formatting, especially to make the type hinting prettier in function definitions.

One argument I had against doing this for the entire code base was that the git blame would be affected, but turns out they've thought about this: https://black.readthedocs.io/en/stable/guides/introducing_black_to_your_project.html

So +1 from me for black formatting for pastas!

@dbrakenhoff dbrakenhoff added the enhancement Indicates improvement of existing features label Jan 9, 2023
@martinvonk martinvonk mentioned this issue Jan 9, 2023
4 tasks
@martinvonk
Copy link
Collaborator

The need to follow up on this change is lower because I formatted the lines affected by type hinting (#441) by hand🙃. However, I am in favor of formatting Pastas with black. Maybe we could format the code with black in a separate branch to see how much of the code is affected and how much this messes up git blame. Maybe it is not as much as we fear and we can make a better assessment of the implications.

@raoulcollenteur
Copy link
Member

+1 for black and automating this through GH actions if possible.

@raoulcollenteur
Copy link
Member

raoulcollenteur commented Jan 10, 2023

Before this issue can be closed:

@dbrakenhoff
Copy link
Member Author

Correct, line-length is set to 88 by black by default.
GitHub Actions will be set in a separate PR/commit.

FYI, for locally ensuring the git blame is rendered correctly (ignoring this commit):

git config blame.ignoreRevsFile .git-blame-ignore-revs

08fe3d7 add mention of black code formatting to docs

@dbrakenhoff
Copy link
Member Author

I can add linting with Black step (replacing the old flake8 step) which checks whether code is black formatted prior to running tests.

We can automate the black formatting and commit it directly to a PR (https://peterevans.dev/posts/github-actions-how-to-automate-code-formatting-in-pull-requests/) but this does not work on forks, so not sure how useful that is. Running black is very easy, so I'm ok with keeping that manual step... but let me know what you think.

@martinvonk
Copy link
Collaborator

martinvonk commented Jan 11, 2023

- [ ] PEP8 compliant code

Maybe we can change this to

  • formatt code with black

and close this issue?

raoulcollenteur added a commit that referenced this issue Jan 13, 2023
Add black formatting (#448) and removing NB output to PR list.
@raoulcollenteur
Copy link
Member

I agree with running black formatting locally now. PR's will fail anyway when it is not black formatted.

@martinvonk I changed the template, so following your suggestions I am closing this Issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Indicates improvement of existing features
Projects
None yet
Development

No branches or pull requests

3 participants