Make further use of Pluralize in the GAP library

[wilfwilson]

This is a follow-on pull request #4050, although it doesn't require that PR.

I describe the changes below, but as mentioned below, this PR will break the tests of some packages in a way that might be annoying/time consuming/tricky to fix, so it might not be worth ever merging this. However, I've made the commits, and so I may as well at least put them here as a PR (if for no other reason than to save anyone else making an independent PR to do this).

1. Use Pluralize in semigroup ViewStrings rather than manually constructing the correct pluralisation

This reduces code duplication slightly.

Unfortunately, this will break the tests in (at least) the Semigroups package. The current ViewString behaviour adds control characters around the word generator{s}, i.e. \>generators\<, but Pluralize does not do this. Perhaps someone more knowledge with control characters than me can tell me which of these behaviours is preferable. The result of this difference can sometimes be seen with the final "n generators" in the ViewStrings here:

In master:

<Rees matrix semigroup 2x1 over <transformation semigroup of degree 2 with 2 
  generators>>

With this PR:

<Rees matrix semigroup 2x1 over <transformation semigroup of degree 2 with 
  2 generators>>

To make this change compatible with the Semigroups package, either a lot of output needs to be suppressed in its tests, or Semigroups needs to test up to whitespace only. There are possibly other, more hacky, solutions.

This may well affect other packages too, so perhaps it is not worth making this change.

2. Use the correct pluralisation of "argument" in the message given for a 'no method found' error

Like the changes #4050, this makes GAP more grammatically correct. Unfortunately, it will break the tests of any package that tests for a 'no method found' error for a function on 1 argument (GAP itself has several such tests, which I update in this PR). In particular, this will break the tests of the Semigroups and Digraphs packages.

It is not see easy to work around this in packages, since 'argument' → 'arguments' is not a whitespace change, and suppressing the output of an error message is neither useful nor possible, as far as I'm aware. You could argue that testing for a 'no method found' error is not a useful test, but I'm not so sure about that.

So again, perhaps it is not worth making this change.

[fingolfin]

I do like this, but it would indeed be best if we knew which packages have tests that are affected by this.

This in turn means it would be useful to have something like what we did in gap-infra again (where we run package tests against master and the latest stable-X.Y branch) -- but perhaps extended to also make it easy for us to run it against other branches. Or, if I may dream, we could even do it here, if we were crazy enough (but we have to be wary as some packages have slow tests, or special requirements; we probably would want to run each package in a separate job. (note that one can generated job matrices with GitHub Actions dynamically: so it'd be possible to first build GAP then get a list of packages from that, then use that to create a build matrix with one job per package!) Of course we would want to avoid building GAP in each job, to keep times minimal; this could be done by having one initial job which builds GAP, tars the resulting binary up and uploads it as an artifact; then later jobs can access that (at least I think that's possible... in any case, I am pretty sure there are ways to make a binary produced by one job available to others).

Hmm, perhaps this rambling should go to its own issue... ;-)

[fingolfin]

As far as I can tell, the changes in the second category ("method not found ... 1 argument(s)") affects exactly semigroups and digraphs, and no other packages.

As to how to deal with this: actually, Test and TestDirectory support passing a custom compareFunction. It wouldn't be too hard to write such a function for Digraphs and Semigroups which deals with these changes... well definitely the one about "1 argument(s)": it could just search & replace on 1 arguments by on 1 argument in both strings and be done with it.

[wilfwilson]

Thanks for the hint.

[fingolfin]

I've modified the PackageDistro CI to allow running tests against arbitrary GAP forks (and arbitrary branches therein, but that was already there). The result can be found here. Summary of failures:

• francy (ignore, it was failing before)
• digraphs
• PackageManager (ignore, it was failing before)
• smallsemi
• semigroups
• recog (ignore, it was failing before)

[wilfwilson]

Thanks for doing that @fingolfin. I was aware that Semigroups and Digraphs would be affected, primarily because of the change from "1 arguments" to "1 argument" in no method found errors (which are often tested in those packages).

The smallsemi failure seems to be caused by the lack control characters given by the output of Pluralize around the word "generator(s)", as described in the message of this PR thread. As you can see from the diff, one of the existing methods adds such control characters, and one doesn't, but with this PR, neither does.

I don't imagine dealing with this will be high on mine or @james-d-mitchell's priority lists, but we might get round to it eventually.

(Also, perhaps Pluralize should be adding control characters around the word it returns, too? I don't understand them and their purpose enough to know...)