[WIP] Black applied

[kumuji]

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure to update the docs?
• Did you write any new necessary tests?
• If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

#1610

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[kumuji]

For some reason CodeFactor started to work. Previously all of the errors were ignored. So it found 25 more issues

[awaelchli]

is this not going to introduce many merge conflicts? (for pending PR's I mean)

[kumuji]

is this not going to introduce many merge conflicts?

It should, I am referring to #1610 (review) . If I understood the comment correctly

[awaelchli]

For some reason CodeFactor started to work. Previously all of the errors were ignored. So it found 25 more issues

It looks like codefactor is only running on changes, and since you change every file, it will now give the warnings

[Borda]

let's ignore all " <-> ' changes

[kumuji]

let's ignore all " <-> ' changes

That's one of the points of black formatter. It is applying standard on the entire file, and you can't specify config. Which means the code will be always the same from project-to-project, commit-to-commit and user-to-user. So far in the repo there is ' and ", there is no clear idea where to use which one. And black will solve it.

So if it would be ignored - there is no point to add black at all. So there would be no point of using pre-commit and formatting check -it will always fail. The only thing that can be specified is on which files to run it, but what is the point of applying it only to some files?

[Borda]

let's ignore all " <-> ' changes

That's one of the points of black formatter. It is applying standard for all of the files, and you can't specify config. Which means the code will be always the same from project-to-project, commit-to-commit and user-to-user. So far in the repo there is ' and ", there is no clear idea where to use which one. And black will solve it.

it is not true, this can be configures as well as you could ignore it when you run black on the whole repo

black --skip-string-normalization

Black also say Black prefers double quotes (" and """) over single quotes (' and ''') which is just black option, not a rule, right?

So if it would be ignored - there is no point to add black at all. So there would be no point of using pre-commit and formatting check -it will always fail. The only thing that can be specified is on which files to run it, but what is the point of applying it only to some files?

I believe that block is not only about ' and ", otherwise it has no added value...

[kumuji]

let's ignore all " <-> ' changes

That's one of the points of black formatter. It is applying standard for all of the files, and you can't specify config. Which means the code will be always the same from project-to-project, commit-to-commit and user-to-user. So far in the repo there is ' and ", there is no clear idea where to use which one. And black will solve it.

it is not true, this can be configures as well as you could ignore it when you run black on the whole repo

black --skip-string-normalization

Black also say Black prefers double quotes (" and """) over single quotes (' and ''') which is just black option, not a rule, right?

So if it would be ignored - there is no point to add black at all. So there would be no point of using pre-commit and formatting check -it will always fail. The only thing that can be specified is on which files to run it, but what is the point of applying it only to some files?

I believe that block is not only about ' and ", otherwise it has no added value...

--skip-string-normalization is fair. Haven't used this option before. Now i see that it is not the rule, but preference of " over '. But isn't it just easier then to just apply it once fully using the recommended option and just forget about this problem later completely?

[codecov]

Codecov Report

Merging #1967 into master will not change coverage.
The diff coverage is 77%.

@@          Coverage Diff           @@
##           master   #1967   +/-   ##
======================================
  Coverage      86%     86%           
======================================
  Files          75      75           
  Lines        4705    4705           
======================================
  Hits         4064    4064           
  Misses        641     641           

[Borda]

--skip-string-normalization is fair. Haven't used this option before. Now i see that it is not the rule, but preference of " over '. But isn't it just easier then to just apply it once fully using the recommended option and just forget about this problem later completely?

let's me express myself (this is not all core/lightning opinion) I think that we still shall let contributors keep their freedom (it is not always easy and pleasant to change some coding habits) in things which are not mandatory, at this moment the content and fast iteration is more important than cool formatting... we do not want anyone to discourage by forcing to code the way he/she does not like/want...

We have some strict/mandatory formatting rules introduced by PEP8 (checked by flake8) the black style is just nice to have and we did (you adding e.g. the pre-commit) maximum for contributors to use it... and thank you for your initiative in this direction 🦝

[Borda]

so maybe check it there is something like pep8speaks also for black?

[jeremyjordan]

@Borda I also typically use ' when developing, but simply let Black correct this to " (I usually use Black to format every time I hit save on a file - this is easy to set up in VS Code). If we're using Black, I would prefer to keep the default style.

[williamFalcon]

if we have to set a default, then ' over "

[mergify]

This pull request is now in conflict... :(

[kumuji]

if we have to set a default, then ' over "

As I understand, it will just ignore the difference between ' and ". Or, by default, make everything to "

[williamFalcon]

@kumuji can you rebase?

[Borda]

if we have to set a default, then ' over "

As I understand, it will just ignore the difference between ' and ". Or, by default, make everything to "

Let's ignore it and remove this diffs from this PR

[mergify]

This pull request is now in conflict... :(

[mergify]

This pull request is now in conflict... :(

[mergify]

This pull request is now in conflict... :(

[awaelchli]

@kumuji PEP bot complains about a missing whitespace after I applied black to a file here in this
#1773 (comment)
Is it normal? It does not seem to show up here.

[kumuji]

@kumuji PEP bot complains about a missing whitespace after I applied black to a file here in this
#1773 (comment)
Is it normal? It does not seem to show up here.

Sometimes it is happening, but I couldn't track why. But in many repos E231 is also added.

[awaelchli]

@kumuji ok, seems to be a bug then. However, I would vote against adding E231 as exception, it's what you meant I guess.

[f4hy]

One thing we should make sure to do is to have this commits marked with --ignore-rev so it is skipped in git blame. This can be added to a .git-blame-ignore-revs file.

[awaelchli]

@kumuji @f4hy I am curious, is there a pre-commit hook or something similar, such that everytime we merge a PR into master, right before that it will automatically run black. Then we wouldn't need to review formatting changes on every PR and just let black do it once we merge? That's a feature I could see useful but no idea if this creates problems.

[f4hy]

@kumuji @f4hy I am curious, is there a pre-commit hook or something similar, such that everytime we merge a PR into master, right before that it will automatically run black. Then we wouldn't need to review formatting changes on every PR and just let black do it once we merge? That's a feature I could see useful but no idea if this creates problems.

I don't think Github actions are allowed to modify code. Looks like this project attempted to do what you wanted but it does not work.

We can offer a shell script that would install a pre-commit hook for developers so that their code is formated on commit. so when they make the PR it will automatically be formated. It would require people to install that hook locally though. After the update, I made in #2898 the only action necessary is to run black with no arguments from the root of the repo. Could even include that in the .run_local_tests.sh to run black maybe?

[awaelchli]

Yes I thought about that, and I would also personally use black if not for the annoying problem that when it formats the whole file even if you just made a few changes in some section. This makes it harder to review the PR when so much whitespace was changed.

[f4hy]

Yes I thought about that, and I would also personally use black if not for the annoying problem that when it formats the whole file even if you just made a few changes in some section. This makes it harder to review the PR when so much whitespace was changed.

Ya, I think thats why a single on time format and enfrocing the format for all future PRs is good. It removes that issue for everything after that point.

[Borda]

We can offer a shell script that would install a pre-commit hook for developers so that their code is formated on commit. so when they make the PR it will automatically be formated. It would require people to install that hook locally though.

that may be tricky as not all people would use and those who will may have much large diff cased formatting changes...
Moreover, there can be problem with collisions as two users would create PR from the master with some missing formatting, then after the first PR merged the other would have conflict... the GH conflict/autorebase is not as good as local so you would need to solve it manually...

Could even include that in the .run_local_tests.sh to run black maybe?

There is no benefit of it as it takes a couple minutes and even if it fails there is blocker for a user to commit his change...

[Borda]

Ya, I think thats why a single on time format and enfrocing the format for all future PRs is good. It removes that issue for everything after that point.

we have found some Black rules quite silly, is there a way how to ignore some?

[kumuji]

@kumuji @f4hy I am curious, is there a pre-commit hook or something similar, such that everytime we merge a PR into master, right before that it will automatically run black. Then we wouldn't need to review formatting changes on every PR and just let black do it once we merge? That's a feature I could see useful but no idea if this creates problems.

I think we already have .pre-commit-config in the repo. So in order for pre-commit hooks to work, people will have to install it first with:

python -m pip install pre-commit
pre-commit install

It will add pre-commit hook which will run black on the code. And later we have github action to verify if code is black formatted.

Ya, I think thats why a single on time format and enfrocing the format for all future PRs is good. It removes that issue for everything after that point.

we have found some Black rules quite silly, is there a way how to ignore some?

We already have --skip-string-normalization and 120 symbols per line

[Borda]

I found some of their statements false like "it minimizes diffs" as with just adding or removing a single argument it changes the function single line to multiline and back...
Also, they close this discussion, seems they just don't want to hear about their own limitations... psf/black#1178 (comment)