Standardize the style of the whole code

[scarlehoff]

My proposal is to do one single commit that standardize the style of the whole codebase.
This simplifies many things, such as not having to look for whitespaces with pylint and then fix them one by one.

It also means I can automatize things on my side without having to turn off linting in a file-by-file (or repo-by-repo) basis.

RE git-blame

Doing it in a single commit means that git blame will still work but also github!, it's a new thing but adding commits to .git-blame-ignore-revs means they get ignored. Yey!

How

I would suggest doing black -l100 for all files and use whatever the default is.
After that we can use a pre-commit hook as @alecandido suggested (or pre-merge, whatever).

Who

Of course some changes might be stupid (like strings that were previously broken in awkward ways). I can volunteer to check the changes one by one.

[RoyStegeman]

I would be happy with this, but if I remember correctly we had a similar discussion a few months ago where you were against adding dependencies of this sort (i.e. black)? Anyway, seems quick enough to do and it would improve some things, so i'm in favor of the idea.

[scarlehoff]

I'm against using pre-commit hooks and I think everybody should just run pylint/black/whatever-they-prefer on their own and fix things by hand but I realize this is quite a big codebase and a moderate-sized team and between the current state or complete automation I've come to prefer the later.

Edit: plus this new development that removes one of the issues with hooks and automation https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/

[RoyStegeman]

I think everybody should just run pylint/black/whatever-they-prefer on their own and fix things by hand

I can't blame you, I also like to daydream sometimes

between the current state or complete automation I've come to prefer the later.

Fair enough. I agree.

[alecandido]

I think everybody should just run pylint/black/whatever-they-prefer on their own and fix things by hand

It looks like you trust a lot yourself.
I'm the opposite, that's why I prefer full automation even in solo projects. But of course, without the least over-head as possible (pre-commit hooks are quite a good compromise).

[scarlehoff]

It looks like you trust a lot yourself.

Actually it's the opposite, the problem is that I don't want to get used to these things happening automatically but rather force myself to write the code correctly in the first place. And I know I am lazy and get used to these things quite quickly.

[alecandido]

I'm just used and addicted. At this point, there is no way back for myself.

[Zaharid]

I have little enthusiasm for this. I am all for using black on new code by default but to me that is much different from bulldozing the old code with it. Most of the codebase predates black by several years so this is going to cause havoc everywhere, more than it can be easily reviewed. While black is okay overall and the current guidelines state that it should be used, I have seen several people, most conspicuously including the OP, complaining about specific formatting choices and requesting them to be adjusted manually in pull requests. While a new project may make these instances opt out by adding ugly formatting comments where required, I think the ship has sailed for this one.

In particular black is not great when it comes to heavy mathematical expressions or pandas-style indexing. So as we saw recently, in many instances it makes the formatting worse than the one that was chosen manually and consciously. The usual arguments of "uncompromising" and "consistency" that you read on hacker news do nothing to sway me and much to make me want to get out of the cult.

While one can ignore revisions in git blame, that still requires everyone to take manual action in every git clone they have. Similar for various hooks, which people can use themselves if they like. Personally I dislike the trends of adding layers and layers of tooling that have no impact on the functionality of the code (or can introduce subtle bugs like when sorting imports). I would much rather have people worrying about other little things like e.g. #1535 which instead make life better notably over time when implemented.

[scarlehoff]

The problem is that if we change for instance config.py or core.py (which are files that are changed quite a lot since they are at the core of vp) we need to format everything by hand since using black would change the entire file.

While black is okay overall and the current guidelines state that it should be used

The problem is that in practice it cannot be used.

complaining about specific formatting choices

Tbf, In the last few PR that I've reviewed the specific formatting choices I've complained about is to make them like what the output of black would've been. But I admit I've been less than consistent in the past.

While one can ignore revisions in git blame, that still requires everyone to take manual action in every git clone they have

Most of the time I use blame in github and that would be automatic but ok, point taken.

Another possibility is to make the pylint bot you used to have into a github action that it is run against every PR with a little script to make sure it only complains about lines that have changed.

Personally I dislike the trends of adding layers and layers of tooling that have no impact on the functionality of the code

I used to agree with this (and still do, just not in this particular case).

I would much rather have people worrying about other little things like e.g. #1535

You can see we are worrying about many little and bigger things if you look at the list of PR.

Anyway, I would like some kind of unanimity in order to change the status-quo, if there's no compromise solution we'll do nothing.

[alecandido]

I get your point, but I still see no reason not to recommend hooks.

Hooks require installation by all the users, and pre-commit in particular really acts incrementally: files not already touched by the commit are not affected by the hooks.
Moreover, it's always simple to step out if you don't like the proposals made by the hooks, since you have to manually stage yourself. Since before committing you're supposed to having staged your stuffs, reverting it's just a matter of:

git restore .
git commit -n

I would explicitly recommend, but a recommendation is not a duty.

[alecandido]

And in any case, even the other choice is not strictly better: while you get rid of unnecessary tooling, you get polluted with formatting requests by reviewers, and you pollute the diffs in the first place.

You can advocate for your specific faith and flavor of purity, but issues are arising the same (that's why this issue has been opened in the first place). I'd recommend a compromise.

[Zaharid]

The problem is that if we change for instance config.py or core.py (which are files that are changed quite a lot since they are at the core of vp) we need to format everything by hand since using black would change the entire file.

While black is okay overall and the current guidelines state that it should be used

The problem is that in practice it cannot be used.

So the main argument seems to be that because black refuses to add an obvious feature out of absolutist spite psf/black#134 we have to disrupt everything to accommodate for that. My answer to that would be to look at the various alternatives posted in the comments of that black issue (and maybe point to them in the documentation). Personally I just tell vim to do '<,'> !black -qS - after maybe after adding some bogus code to match indentation levels.

[scarlehoff]

So the main argument seems to be that because black refuses to add an obvious feature out of absolutist spite psf/black#134 we have to disrupt everything to accommodate for that

No, the main argument is that it would be nice that, since we are many people working on the same code, we standardize said code and it would be even nicer if we have some automatic measures to standardize also future code.

I would be content with the first half but just to reiterate, if there's no compromise solution we do nothing (and we have an issue to point to if anyone ask the same question in the future I guess).

with formatting requests by reviewers

In all fairness there I was genuinely curious whether the Mac version of code just eliminates blank lines at the end.

[Zaharid]

So the main argument seems to be that because black refuses to add an obvious feature out of absolutist spite psf/black#134 we have to disrupt everything to accommodate for that

No, the main argument is that it would be nice that, since we are many people working on the same code, we standardize said code and it would be even nicer if we have some automatic measures to standardize also future code.

I would be content with the first half but just to reiterate, if there's no compromise solution we do nothing (and we have an issue to point to if anyone ask the same question in the future I guess).

I'd say the first part is taken care of with the existing node in the docs that new code should use black (which is definitively possible when appending to existing files).

with formatting requests by reviewers

In all fairness there I was genuinely curious whether the Mac version of code just eliminates blank lines at the end.

I am not sure what would be gained here: Now you have to tell new contributors to use black. If we had hooks you would have to tell new contributors to use hooks. And answers questions about how to install them and them not working. And at some point we decide to make black a hard dependency of the build process and get surface area for issues like this psf/black#2964, which if I have to extrapolate from the effort it took to fix in projects I am involved in this week, I would say it may well have had a cost in the millions.

Btw here is an old discussion for comedic effect #512 (comment)

[scarlehoff]

Btw here is an old discussion for comedic effect #512 (comment)

Oh, how the tables have turned 😆

[Zaharid]

@alecandido I would be fine with a script of some sort that applies darker (assuming it works fine, have never used it) along with relevant documentation. Assuming it stays strictly optional and out of the loop. I'd be unlikely to use it myself but it might help other people.

[alecandido]

I'd prefer to be able to format on save with black, but I'm not the main contributor of validphys for the time being, so if it's up to someone else, I'd be happy with darker as well (at least it will start lowering the barrier for the time in which I'll apply black on whatever file).

Just to mention it: your script is called pre-commit https://github.com/akaihola/darker/blob/master/.pre-commit-hooks.yaml, once more.

[RoyStegeman]

I think this is closed by #1710, with the agreement that we don't make a single commit for the entire repo but ask people to run black and isort on the changed files before merging a PR.