Align PrintString for empty lists with PrintObj, ViewString and ViewObj

[zickgraf]

PrintObj, ViewString and ViewObj all return/print two spaces.

I fear that this change might break too many packages but I wanted to bring it up for a quick discussion.

Text for release notes

String and PrintString of empty lists now contain two empty spaces

[fingolfin]

Why do you think it would break many packages?

Anyway, for now it breaks tst/testinstall/strings.tst ;-). Once that is tested and CI passes here, I can run the PackageDistro CI on this PR to find out if / which packages might be affected by this.

[fingolfin]

Initiated PackageDistro CI run against this PR here: https://github.com/gap-system/PackageDistro/actions/runs/4494342841

[fingolfin]

Possible patch:

commit 496cbffd626a47044a941fdcfdb946127854a780
Author: Max Horn <max@quendi.de>
Date:   2023-03-22 23:32:22 +0100

    Adjust tests

diff --git a/tst/testinstall/strings.tst b/tst/testinstall/strings.tst
index d0dd5388b..bb6bedcf0 100644
--- a/tst/testinstall/strings.tst
+++ b/tst/testinstall/strings.tst
@@ -130,9 +130,9 @@ gap> DisplayString(x);
 gap> ViewString(x);
 "[  ]"
 gap> PrintString(x);
-"[ ]"
+"[  ]"
 gap> String(x);
-"[ ]"
+"[  ]"
 
 # Dense list
 gap> x:=[1,2,3];

Since you disabled the "Allow edits and access to secrets by maintainers" checkbox, I can't amend the PR with this fix. The PackageDistro CI then will fail, as it runs testinstall. So I'll wait for you to update the PR

[zickgraf]

Why do you think it would break many packages?

FinSetsForCAP uses PrintString of lists for constructing view and display strings which appear in test files. I expect other packages to do similar things. But let's see.

Since you disabled the "Allow edits and access to secrets by maintainers" checkbox, I can't amend the PR with this fix. The PackageDistro CI then will fail, as it runs testinstall. So I'll wait for you to update the PR

Ah, sorry, I usually want to keep it enabled for GAP PRs but always forget about it and disable it by habit. Anyway, the PR is updated (and the checkbox for allowing edits now set). Thanks for triggering the PackageDistro runs!

[zickgraf]

The CI failure seems to be due to a race condition with the update of https://github.com/gap-system/PackageDistro/releases/tag/latest

[fingolfin]

New CI run at https://github.com/gap-system/PackageDistro/actions/runs/4526637539 reports failures in digraphs and semigroups

[zickgraf]

New CI run at https://github.com/gap-system/PackageDistro/actions/runs/4526637539 reports failures in digraphs and semigroups

Thanks for checking! I have looked at the diffs, those look totally manageable to me. I'm happy to provide PRs for the two packages if this PR gets merged.

[fingolfin]

I am not sure if those packages would be happy to ditch CI tests on older GAP versions, though...

[zickgraf]

I thought about adding the following compatibility code to the packages:

# forward compatibility with GAP 4.13
# see https://github.com/gap-system/gap/pull/5418
if not CompareVersionNumbers( GAPInfo.Version, "4.13" ) then

    InstallMethod( String,
        "for a string (do nothing)",
        [ IsString and IsEmpty ],
        function(s)
          if not IsStringRep(s) then
            return "[  ]";
          else
            return s;
          fi;
        end);

fi;

Or the tests in those packages could (temporarily) ignore whitespace changes.

Do you think one of these could be a sensible course of action?

[ChrisJefferson]

This sort of thing is annoyingly hard to fix. You could ask if those packages are happy to ignore whitespace issues (they might care about exact formatting), or even splitting the tests based on GAP version.

You can write something like this in a test (sorry, not tested)

#@if CompareVersionNumbers(GAPInfo.Version, "4.13")
...
#@else
....
#@endif

[zickgraf]

Thanks for the suggestion @ChrisJefferson! I have created PRs for Digraphs and Semigroups with a slightly different approach which can be merged independent of this PR. I hope this simplifies things.

[james-d-mitchell]

@zickgraf I'll be happy enough to merge the PR's you've raised in Digraphs and Semigroups, but I'm reluctant to do that before this PR is merged. I'll mention this in those PR's also

[zickgraf]

Thanks for the feedback, @james-d-mitchell!

Then this PR should be ready for merge, and afterwards the PRs in Digraphs and Semigroups could be merged. If this could still make it into the GAP 4.13 release, I would be very happy, because it would keep the transition period short (at least for packages always requiring the latest version of GAP). (Sorry for bringing PRs up this late in the release cycle, I was occupied with my thesis until yesterday.)

[zickgraf]

I have rebased to re-trigger the CI, all checks are successful now.

[fingolfin]

@james-d-mitchell @zickgraf the patches to digraphs and semigroups are backwards compatible, aren't they? So the "normal" order would be for the packages to be updated, then this PR merged, to prevent breaking CIs.

But if you don't mind having your package CI tests broken for a time when testing against GAP master... Oh and of course the PackageDistro tests for semigroups and digraphs also will report failure for a time (but I can ignore that).

[fingolfin]

Fine by me.

(I think it would be nicer to standardize all to [ ] with a single space, but I realize this might require a lot more work, and what's in this PR is at least consistent.)

[fingolfin]

I've put it on the 4.13.0 milestone. Will wait a little bit longer with merging, though.

[zickgraf]

Thanks @fingolfin! Yes, the patches to digraphs and semigroups are backwards compatible, so the order in which the PRs are merged should not matter.

[fingolfin]

@james-d-mitchell since this was merged it'd be great if digraphs and semigroups could be updated as well :-)

[james-d-mitchell]

@fingolfin Sure, I can make another release just now

[james-d-mitchell]

New versions of Semigroups and Digraphs released, thanks for the input @fingolfin and @zickgraf