Repository navigation
Whitespace fixes - #1498
Whitespace fixes#1498ulgens wants to merge 4 commits into
Conversation
ilyamandel
left a comment
There was a problem hiding this comment.
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.
|
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. |
|
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. |
03ef39d to
0aaf0eb
Compare
Fixing whitespace-related issues using
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.