Re-style codebase: validphys

[scarlehoff]

In this PR I've run black and isort against most of validphys.

I've gone through all changes. I've added fmt: off in a couple of places where I didn't like the changes that black did and fixed several strings (mostly those that ended up looking "like" "this").

I've skipped the tests and the scripts since they are in a category of their own and I prefer to hace some pre-approval of the people involved before spending more time looking at infinite diffs.

I've done validphys first because as far as I can see there are no big PRs left touching vp (other than the commondata parser, but that's on me). We could leave out specific files (by putting fmt: off in) that people might want to keep unchanged because they are working on them.

[RoyStegeman]

why did you put this here? I would agree that I don't especially like what black does with these long lines but also don't really think it's that much of a problem.

[scarlehoff]

I didn't like the changes. Like it is right now they are ordered as

   = a1*b1
   + a2*b2

which makes sense to keep imho

[RoyStegeman]

Good enough for me

[scarlehoff]

@Zaharid are you ok with merging this as is in order to minimize possible conflicts?

[Zaharid]

Haven't gone over the changes in detail, and I won't. Ideally someone would.

In terms of style changes, I am hard pressed to see anything that is an actual improvement with most of the things being churn, which is why I feel at best lukewarm about all this. I guess it's good that we don't also get churn from string quotes. With that said, I suppose now that ignore revision files are more standardized my main reason for strongly opposing this is diminished. Please do add documentation about the ignore revision file, and how to use it to run git blame locally (ideally as a part of a different PR).

Relatedly, I wish various offending commits (in the photon pr) were squashed out of existence rather than added to that file. This really should be for running the formatting changes we want. So the docs should say to not do that any more.

[RoyStegeman]

In terms of style changes, I am hard pressed to see anything that is an actual improvement with most of the things being churn, which is why I feel at best lukewarm about all this.

It's not so much a style improvement as it is an easy way to keep a consistent style among all developers. For example

I wish various offending commits (in the photon pr) were squashed out of existence rather than added to that file.

I agree that this is what I would have done, but at some point I have to weight the benefits of requesting yet another change with the cost of requesting/discussing and making that change, and in this case I decided to leave it be. Similar things happen with the code styling (though I know you're less lenient about styling than I am). I think a benefit of using black is that reviewers don't have to waste time nitpicking about formatting and yet the style of the code will remain consistent.

As a more practical comment: the photon PR commits are now in master and I don't think we should get in the habit of force-pushing master.

Please do add documentation about the ignore revision file, and how to use it to run git blame locally (ideally as a part of a different PR).

git blame reads the ignore file by default, even in the browser.

[Zaharid]

In terms of style changes, I am hard pressed to see anything that is an actual improvement with most of the things being churn, which is why I feel at best lukewarm about all this.

It's not so much a style improvement as it is an easy way to keep a consistent style among all developers. For example

I did not argue that.

I wish various offending commits (in the photon pr) were squashed out of existence rather than added to that file.

I agree that this is what I would have done, but at some point I have to weight the benefits of requesting yet another change with the cost of requesting/discussing and making that change, and in this case I decided to leave it be. Similar things happen with the code styling (though I know you're less lenient about styling than I am). I think a benefit of using black is that reviewers don't have to waste time nitpicking about formatting and yet the style of the code will remain consistent.

As a more practical comment: the photon PR commits are now in master and I don't think we should get in the habit of force-pushing master.

I dd not argue that.

Please do add documentation about the ignore revision file, and how to use it to run git blame locally (ideally as a part of a different PR).

git blame reads the ignore file by default, even in the browser.

I don't believe that is true.

[RoyStegeman]

I don't believe that is true.

You don't have to take my word for it, just go to the "blame view" of any file and it shows a message that the ignore-revs file is taken into account.

[Zaharid]

I understand it works in github, but it does not if you run git blame locally. For that you need to either pass a flag or set a config option in git.

[RoyStegeman]

Right that could indeed be added to the docs

[scarlehoff]

I think people using git blame from the command line know how to use man or - - help. I think the docs should just tell people to use the tools and that when many things change contain it to a single commit that can get ignored.

[Zaharid]

I think people using git blame from the command line know how to use man or - - help. I think the docs should just tell people to use the tools and that when many things change contain it to a single commit that can get ignored.

--help doesn't tell you what the naming convention for the ignore file has been chosen for this particular project and ideally you wouldn't have to cross reference a bunch of different docs for git, github and so on.

Overall the problem with adding a mountain of tools for your project is that then you have to support them and document them and ideally know which ignore file gets read by which tool with which flag.