[ulgens]

Fixing whitespace-related issues using

• https://prek.j178.dev/0.5.2/builtin/#end-of-file-fixer
• https://prek.j178.dev/0.5.2/builtin/#trailing-whitespace

Modern IDEs tend to apply these fixes whenever you edit the related lines / file, which causes noise and pollutes unrelated commits.

The diff is not small, but being one of the most basic git hooks - styling fixes, I believe this is important to have.

[ilyamandel]

I realise that all the changes should be very benign, but I am still a bit apprehensive to apply this to so many files at once, where we clearly don't have the capacity to manually check... I'll leave it to @jeffriley to make the call, since some of the affected files include core C++ code.

[jeffriley]

I'm all for a consistent style, but all the C++ files will be replaced by the new version soon - some files will disappear entirely, and new files will be added. I'm not sure applying these style changes to the C++ files now is worth the effort of reviewing the changes (I agree with @ilyamandel that the changes should be benign, but I'd bet that everybody that submits a PR believes the changes are good... so they should be reviewed - and even a cursory review of 268 files is a bit onerous). And as far as changes to the markdown documentation files go - yes, the changes should be benign, but someone needs to look at the result to confirm that.

Maybe do each type of file as separate PRs - that would reduce the onerous nature of a giant PR, and enable people to review files that suit their expertise (e.g. @ilyamandel for matlab files).

I'm out for 3 weeks or so in about a week, so I couldn't even start looking at this until the end of November or so.

EDIT: I forgot to add:

I'm not sure the problem fixed by this PR actually hurts us - having a tab or one or more spaces at the end of a line (especially if it's a comment) doesn't seem to me to be terribly problematic. Sure, if there are so many spaces at the end of a line that it affects how the file is displayed in an IDE, that's an issue - but we'd pick that up in a PR review wouldn't we? Same for too many blank lines at the end of a file. Maybe automating removing those things is useful, but only if it doesn't cause us more work - if we can prove that the automation always produces benign results (so that we don't have more reviewing work), then I'm ok with adopting it.

[avivajpeyi]

Scrolling through the changes -- i think they are mostly as advertised...

But this will indeed be hard to review... this is partly why we never added automated whitespacing fixing/linting in the precommit file 😅

I would probably wait till Jeff's new version is ready, and maybe we can get jeff to install and run the linting stuff for the new version before he does all his testing.

[ilyamandel]

@ulgens -- based on the comments above, I don't think this is worth the risk; the benefits appear very small, and don't justify even the very low risk of something going wrong after a change. I suggest we abandon this PR.

[ulgens]

but I am still a bit apprehensive to apply this to so many files at once, where we clearly don't have the capacity to manually check...

I can split the PR for file types or folders etc.

but all the C++ files will be replaced by the new version soon - some files will disappear entirely, and new files will be added.

I guess they will need some formatting too, I'm missing why it's a bad thing.

yes, the changes should be benign, but someone needs to look at the result to confirm that.

A very easy way of doing it is amending the commit without committing it. You can revert the changes, apply the rule yourself and validate that everything is clear in just a couple of seconds.

I'm not sure the problem fixed by this PR actually hurts us

Right now, the experience of editing a file in the repo with a modern IDE feels like you are fighting againist the IDE and limiting a PR to only your changes requires constant attention. The noise created by that struggle already exist in the repo.

but we'd pick that up in a PR review wouldn't we? Same for too many blank lines at the end of a file.

I'd say so, but the repo already has multiple examples where it wasn't the case.

Maybe do each type of file as separate PRs - that would reduce the onerous nature of a giant PR, and enable people to review files that suit their expertise (e.g. @ilyamandel for matlab files).

I'm okay with this option. Would be nice to have these cleaning at least on Python files.

but only if it doesn't cause us more work - if we can prove that the automation always produces benign results (so that we don't have more reviewing work), then I'm ok with adopting it.

I believe this is already the case, but it doesn't feel that way because the tools are new. After a certain back and forth with prek and ruff, one sees that "unsafe" changes are already handled separately and the auto applied changes always safe (benign) by design.

But this will indeed be hard to review... this is partly why we never added automated whitespacing fixing/linting in the precommit file 😅

Wait until ruff format PR 😬

I don't think this is worth the risk

I'm missing the risk, but I agree that the huge PR doesn't convey ease. I'd like to limit the changes to one rule and filetype and move from there.