gap-system / gap

Main development repository for GAP - Groups, Algorithms, Programming, a System for Computational Discrete Algebra
https://www.gap-system.org
GNU General Public License v2.0
813 stars 161 forks source link

trim trailing whitespace? #5689

Open ThomasBreuer opened 7 months ago

ThomasBreuer commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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.