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

Introduce formatter black #652

Merged
merged 3 commits into from
Apr 16, 2019
Merged

Introduce formatter black #652

merged 3 commits into from
Apr 16, 2019

Conversation

tabergma
Copy link
Collaborator

@tabergma tabergma commented Apr 9, 2019

closes #644

  • Introduce formatter black.
  • Travis build will fail if the code is not properly formatted.
  • Update README on how to use black

@tabergma tabergma requested a review from alanakbik April 9, 2019 07:02
@alanakbik
Copy link
Collaborator

@tabergma thanks for adding this! I've looked into black and the auto-formatting style seems to work pretty well. The default line-length of 88 seems very small to me (lots of scrolling + monitor half unused) but this is something that Python people seem to like so that's ok from my side. I'd be happy to try this out.

In case the travis build fails due to not matching black formatting, is there a way to either a) automatically execute black formatting, or b) notify the user what they need to do to pass the checks?

@alanakbik
Copy link
Collaborator

👍

@tabergma
Copy link
Collaborator Author

You cannot fully automate black as far as I know. However, you can use pre-commit: This will reformat files on every commit you do. I added a section to the Readme on how to use it.

Regarding you point b: We could add a Pull Request template that contains some check points, such as "added tests", "reformatted files using black (check Readme)", "updated documentation". This way, the user would get a hint on what he needs to do and could check the Readme for a more detailed description.

@tabergma
Copy link
Collaborator Author

👍

@alanakbik alanakbik merged commit 48476f0 into master Apr 16, 2019
@stefan-it stefan-it deleted the black-formatting branch April 16, 2019 08:00
alanakbik pushed a commit to nlp-pucrs/flair that referenced this pull request Apr 16, 2019
@stefan-it
Copy link
Member

stefan-it commented Apr 16, 2019

What "strategy" should we use for the current opened PRs? The build for each of them will be ok, but after a merge on master, the build will fail. Should we "force" a rebase on master now, or should we file a follow-up PR later 🤔

@tabergma
Copy link
Collaborator Author

I would suggest merging (not rebase) the master into the PR branches. Depending on the size of the PR there might be merge conflicts to solve. But they should be fairly easy to solve. Afterwards, reformat the code again in the PR branch and you should be good to go.

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.

Introduce formatter
3 participants