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

ci: Introduce code formatter #482

Closed
akihironitta opened this issue Dec 25, 2020 · 10 comments · Fixed by #511
Closed

ci: Introduce code formatter #482

akihironitta opened this issue Dec 25, 2020 · 10 comments · Fixed by #511
Labels
ci/cd Continues Integration and delivery discussion enhancement New feature or request refactoring
Milestone

Comments

@akihironitta
Copy link
Contributor

🚀 Feature

We've spent quite some time on fixing code formatting in code reviews by our own hands, but this should be done automatically by a Python code formatter.

List of formatters

Additional context

I have used black with isort (for import formatting) and flake8 (for pep8 check) in Optuna, and it worked really well, so +1 for black.

In PL, black was added in Lightning-AI/pytorch-lightning#1610, but it hasn't been really used... We can start to employ the code formatter here, and if there seems no problem with it, we can enable it in PL later, too.

cc: @Borda @PyTorchLightning/bolts-contributors

@akihironitta akihironitta added the ci/cd Continues Integration and delivery label Dec 25, 2020
@oke-aditya
Copy link
Contributor

I have used black earlier and we can add a simple CI check for it as well.
At places there are few collisions with black and flake8. I'm not sure if both can be together.
Black checks for linting errors too I think.

Also we can add to pre-commit-config.yamlfile present in bolts.

@briankosw
Copy link
Contributor

At places there are few collisions with black and flake8. I'm not sure if both can be together.

They can definitely work together, and black themselves published suggestions on how to make black work together with other tools such as flake8 and isort.

I think using black will significantly increase pl_bolts' formatting (and therefore code quality), so I'd be happy to help in anyway with adding this to the current array of CI tools.

@oke-aditya
Copy link
Contributor

I read the above black page, I think it will work fine as they have mentioned.
They have also mentioned how to use with precommite here
and Github CI action here

Plus add it to readme badge 😄 like this

@akihironitta
Copy link
Contributor Author

akihironitta commented Dec 28, 2020

@oke-aditya @briankosw Thank you both for sharing the useful information!
So, as I read your comments, I believe we should do the followings.

To do

  1. add black to pre-commit hooks
  2. add black to GitHub Actions
  3. update flake8 and isort configs to avoid conflicts with black
  4. add the badge to readme

Let's get to work on this once we have positive comments from other core members :]

@akihironitta
Copy link
Contributor Author

akihironitta commented Jan 9, 2021

@Borda @PyTorchLightning/bolts-contributors Shall we introduce black so that we can easily check formatting including trailing-whitespaces in the pre-commit, and we don't need another PR like Lightning-AI/pytorch-lightning#5387 in the future?
Would be reasonable to ntroduce black here in Bolts first so that we can make sure that black works with other checkers and we can apply this to PL later in the future.

@Borda
Copy link
Member

Borda commented Jan 9, 2021

we can easily check formatting including trailing-whitespaces

I think that we can do this check with flake8 right?
but yes, lest try black here :]

UPDATE: well just check the listed yapf and it looks better, seems to be more flexible and particular I like COALESCE_BRACKETS
see: https://www.kevinpeters.net/auto-formatters-for-python

@akihironitta
Copy link
Contributor Author

@Borda I've just noticed your updated comment (though I already created a draft PR #507 to add black).

I agree that COALESCE_BRACKETS of yapf looks neat! Using black would be easier since it is widely used and has fewer parameters (and I've used it before), but we could use yapf if you'd like :]

@oke-aditya @briankosw Please leave your comments if you've got any.

@akihironitta akihironitta mentioned this issue Jan 9, 2021
8 tasks
@Borda
Copy link
Member

Borda commented Jan 9, 2021

Personally I would prefer flexibility over mainstream... I would not be affraid of using yapf since it Facebook in house tool...

@oke-aditya
Copy link
Contributor

oke-aditya commented Jan 9, 2021

I have used black. I do not like it much. it makes code too lengthy and is very strict checker, also collides with isort and flake8.

I'm not sure how to configure yapf, how to add CI, pre commit though. I agree with @Borda's suggestion. I would like a flexible formatted (like flake8). Also facebook style would be preferred.

But we need consistency with PyTorchLightning too, it might also need to use yapf. Torchvision uses only flake8 though.

@akihironitta akihironitta changed the title ci: Introduce code formatter black ci: Introduce code formatter Jan 10, 2021
@Borda
Copy link
Member

Borda commented Jan 10, 2021

for the record, this is the proposed config

[yapf]
based_on_style = pep8
spaces_before_comment = 2
split_before_logical_operator = true
COLUMN_LIMIT = 120
COALESCE_BRACKETS = true
DEDENT_CLOSING_BRACKETS = true
ALLOW_SPLIT_BEFORE_DICT_VALUE = false
BLANK_LINE_BEFORE_NESTED_CLASS_OR_DEF = true
NO_SPACES_AROUND_SELECTED_BINARY_OPERATORS = false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/cd Continues Integration and delivery discussion enhancement New feature or request refactoring
Projects
None yet
4 participants