Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trim trailing whitespace? #5689

Open
ThomasBreuer opened this issue Mar 26, 2024 · 6 comments
Open

trim trailing whitespace? #5689

ThomasBreuer opened this issue Mar 26, 2024 · 6 comments

Comments

@ThomasBreuer
Copy link
Contributor

Today I stumbled over a problem due to #5312.
When I saved the file doc/ref/mloop.xml, three trailing blanks got removed automatically.
(git restore seemed to be the only way to get the original contents back.)

Logically, these blanks are needed.
Technically, it does not matter because the GAP session in question is not part of tests.

Is it really a good idea to remove trailing whitespace automatically whenever a file gets saved?

@fingolfin
Copy link
Member

These editor settings are specified in .editorconfig. We already turn off the trimming for .tst files, we could also add an exception .xml files.

@ThomasBreuer
Copy link
Contributor Author

I had expected a lot of trailing blanks in examples inside .gd files, because GAP output contains a lot of them.
Now I noticed that apparently these blanks have been removed some time ago, which causes no problems because the tests for these examples are executed "up to whitespace".

If trailing blanks are so unpopular then wouldn't it be more logical to change GAP such that they are not produced anymore, instead of removing them afterwards, and changing the test setup to ignore them in comparisons?

@fingolfin
Copy link
Member

Changing GAP to avoid these trailing whitespace is difficult because it lacks required information internally to make it easy -- computing in advance where a line break will be is difficult the way things are set up in the kernel. We'd have to do odd tricks like "delay printing of all whitespace until a non-whitespace is printed". This sounds extremely error prone and difficult to me. At the same time, it sounds like a solution for a problem nobody has? It seems to me the current state works reasonably well?

@ChrisJefferson
Copy link
Contributor

One thing I'd be happy to do is switch .tst to ignore whitespace at the end of lines. I'd personally argue (I've used the same thing in other programs) that if I can't "see" the difference between two lines, does the presence or absence of whitespace at the end of lines matter? Then we could (I think?) trim whitespace everywhere?

@ThomasBreuer
Copy link
Contributor Author

Concerning "a solution for a problem nobody has":
Apparently some people had problems with trailing whitespace, because they decided to tell editors to (silently) remove them.

One can call Test either to do exact comparisons or to compare up to whitespace in a broader sense, but if the reference output is already edited then exact comparisons do not make sense at all.

@ChrisJefferson
Copy link
Contributor

To be concrete, my suggestion is to add a new uptotws (up to trailing whitespace) comparison function, and then make that the default. We can add an "exact", for people to opt into the current exact comparision. This will let us not worry about deleting trailing whitespace from tests when saving.

I'm happy to write the PR for that, but obviously it's no good if people don't want it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants