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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

define Yapf config #5591

Merged
merged 8 commits into from Jan 28, 2021
Merged

define Yapf config #5591

merged 8 commits into from Jan 28, 2021

Conversation

Borda
Copy link
Member

@Borda Borda commented Jan 20, 2021

What does this PR do?

eventually, we would move to Yet-Another-Python-Formater so this is just setting rules, for illustration how it goes you can check Bolts codebase when we have already deployed it
With deploy YAPF check we may drop ISORT which has atm incompatibility with PEP8, see PyCQA/isort#1594 (comment)
discussed with @tchaton and finetuned config with @akihironitta

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified
  • Check that target branch and milestone match!

Did you have fun?

Make sure you had fun coding 馃檭

@Borda Borda added the feature Is an improvement or enhancement label Jan 20, 2021
@Borda Borda added this to the 1.2 milestone Jan 20, 2021
@Borda Borda added this to In progress in Code Health via automation Jan 20, 2021
@Borda Borda self-assigned this Jan 20, 2021
@codecov
Copy link

codecov bot commented Jan 20, 2021

Codecov Report

Merging #5591 (6820889) into release/1.2-dev (3da28fd) will decrease coverage by 0%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           release/1.2-dev   #5591    +/-   ##
================================================
- Coverage               89%     89%    -0%     
================================================
  Files                  168     168            
  Lines                12423   12267   -156     
================================================
- Hits                 11103   10933   -170     
- Misses                1320    1334    +14     

@carmocca
Copy link
Member

So why yapf over alternatives? (black, for example: https://news.ycombinator.com/item?id=17155048)

@Borda
Copy link
Member Author

Borda commented Jan 21, 2021

So why yapf over alternatives? (black, for example: https://news.ycombinator.com/item?id=17155048)

We have already spoke about it, black is just one (no way to configure as far as I know) so you either like it or hate it...
On the other hand with yapf you have some freedom to set it up...

Btw, not sure how much it still relevant comment from 2018 (2 years ago)
And my experience is that black is less deterministic and much longer which makes it more difficult to read...

@carmocca have you check how Bolts looks like now?

EDIT: in fact there are not so many alternatives, see Lightning-Universe/lightning-bolts#482

@carmocca
Copy link
Member

And my experience is that black is less deterministic and much longer which makes it more difficult to read...

Never used yapf but reading online, seems like its the other way around.

What I care about the most is that the formatter only allows one consistent single style. If yapf does, then that's OK with me

@Borda
Copy link
Member Author

Borda commented Jan 21, 2021

What I care about the most is that the formatter only allows one consistent single style. If yapf does, then that's OK with me

So far I have tested, with one setting it does, but if you want we can test it a bit longer with Bolts first...

@awaelchli
Copy link
Member

the drawback is that if you change your mind about the config, you have to reformat the whole code base.
I have one wish, I beg you, please if it's possible can we avoid this code style:

foo_long_function_(arg1, arg2, arg3
                   arg4, arg5, arg6)

it leads to uneven indentation which is just horrible in my opinion.

@Borda
Copy link
Member Author

Borda commented Jan 21, 2021

I have one wish, I beg you, please if it's possible can we avoid this code style:

foo_long_function_(arg1, arg2, arg3
                   arg4, arg5, arg6)

Heh, that you are lucky guy, I hate it too =D

Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it ! Great addition !

@Borda Borda enabled auto-merge (squash) January 21, 2021 08:56
@Borda Borda added the ready PRs ready to be merged label Jan 21, 2021
@mergify mergify bot requested a review from a team January 22, 2021 02:44
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great addition !

@akihironitta
Copy link
Contributor

In Bolts, I had a collision between flake8 and yapf. Applying yapf caused the error [W503] line break before binary operator from flake8, so shall we modify the flake8 config to ignore W503? https://github.com/PyTorchLightning/pytorch-lightning-bolts/pull/462/files#r561509665

@Borda
Copy link
Member Author

Borda commented Jan 24, 2021

In Bolts, I had a collision between flake8 and yapf. Applying yapf caused the error [W503] line break before binary operator from flake8, so shall we modify the flake8 config to ignore W503? https://github.com/PyTorchLightning/pytorch-lightning-bolts/pull/462/files#r561509665

we can configure this in yapf, right?

@akihironitta
Copy link
Contributor

@Borda Could you have a look at Lightning-Universe/lightning-bolts#539? I'm having difficulty in resolving the conflicts between pep8 (flake8) and yapf. It seems that yapf is not really compliant with pep8 even if we set:

[yapf]
based_on_style = pep8

@akihironitta
Copy link
Contributor

In Bolts, we had conflicts between yapf and flake8, and @Borda resolved errors from flake8 by writing those lines in a different way. Lightning-Universe/lightning-bolts#539 So, here, I don't think we need to modify our flake8 config for now.

@Borda
Copy link
Member Author

Borda commented Jan 27, 2021

I still think that you shall write reasonable code, and there are always multiple ways... :]

setup.py Outdated Show resolved Hide resolved
Code Health automation moved this from In progress to Review in progress Jan 27, 2021
@mergify mergify bot requested a review from a team January 27, 2021 23:42
@Borda Borda requested a review from awaelchli January 27, 2021 23:59
Code Health automation moved this from Review in progress to Reviewer approved Jan 28, 2021
.yapfignore Outdated Show resolved Hide resolved
@Borda Borda merged commit 99ea2a3 into release/1.2-dev Jan 28, 2021
Code Health automation moved this from Reviewer approved to Done Jan 28, 2021
@Borda Borda deleted the yapf branch January 28, 2021 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement ready PRs ready to be merged
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants