Improve error messages in trans.gi, semitran.gi, semipperm.gi, reesmat.gi

[fingolfin]

These files currently contain error messages like these:

    ErrorNoReturn("IsomorphismTransformationMonoid: usage,\n",
                  "the argument must be a semigroup with a ",
                  "multiplicative neutral element,");

which leads to error messages like this one:

gap> IsomorphismTransformationMonoid(s);
Error, IsomorphismTransformationMonoid: usage,
the argument must be a semigroup with a multiplicative neutral element, at GAPROOT/lib/semitran.gi:360 called from
<function "unknown">( <arguments> )
 called from read-eval loop at *stdin*:2

There are IMHO multiple things wrong with this:

• there is a trailing comma;
• there are newlines in the error message
• the fragment "usage," should be removed
• I'd also argue that the name of the function should be removed -- it should be printed as part of the generic backtrace. However, that seems to be broken in the example above.

Also note that in at least two error messages, there is no space after usage, (namely in OnKernelAntiAction and in NumberTransformation). The latter also occurs in tests -- I wonder why nobody noticed this when generating the test?

[james-d-mitchell]

I guess that I wrote these error messages and the offending tests.

I am not sure I agree that these are "wrong" in this context for reasons I will explain. Before doing that, let me say that I am not particularly attached to the format of these error messages, and am happy for them to be changed, even to change them myself.

I am not sure @fingolfin where the conventions that you seem to want to apply are recorded, perhaps you can direct me?

Looking at the other errors in the library it is/was not clear to me what the conventions for error messages should be: the errors elsewhere in the library are inconsistently formatted, and often uninformative. So, when coming to write these, I probably defaulted to using the conventions I use elsewhere (I am not defending this choice, merely explaining it).

Here are some comments:

• About the name of the function being included: until relatively recently GAP did not report very accurately where errors occurred, and I found it helpful to have the function/method name displayed in the error. Probably this is no longer relevant, but as @fingolfin points out the function name does not appear in the generic backtrace in the example in this issue.

• "there are newlines in the error message", there are according to a quick grep of the library a total of 196 error messages including a newline in the GAP library, only about 20 of them are in the files named in this issue. Why are these files being singled out in this issue, and not the other approx. 176 errors in other files?

  The reason for including this in the first place is that without it the error you mention looks like the following (in a terminal with 80 columns): with words wrapped badly across the lines. The intention was to make the error message more readable, although in this case, one is not clearly better than the other. This would also be improved by removing the IsomorphismTransformationMonoid from the error message.

Error, IsomorphismTransformationMonoid: usage, the argument must be a semigroup w
ith a multiplicative neutral element, at /Users/jdm/gap/lib/semitran.gi:360 call
ed from
<function "unknown">( <arguments> )
 called from read-eval loop at *stdin*:2
type 'quit;' to quit to outer loop
brk>

• "the fragment "usage," should be removed": after a quick grep, there are about 48 error messages in the files you mention that contain the word usage, and there are a further 286 error messages in other files that contain the word usage. Again I am not sure why these 4 files are being singled out in this issue, nor what convention is being applied.

• the issue of trailing comma: I find/found it useful to know when the error message passed to Error and ErrorNoReturn ends and the generic backtrace begins, which is why I included a trailing comma. Probably this is not necessary any longer.

• "I wonder why nobody noticed this when generating the test?" I guess this is a rhetorical question, but in any case, perhaps it was a mistake?

[fingolfin]

@james-d-mitchell Seems I hit a nerve there :-(. Please be assured that I did not intend this as a personal attack on you nor on your code. Let me briefly explain how this report came to be: I am currently working on merging GAP and HPC-GAP library code, which leads to me looking at a huge portion of the GAP library, and investigating test diffs. During that, I noticed by chance some test diffs (caused by using old tests with new library code) which involved one of the error messages this issue is talking about; it was one of those with a missing space after "using,". I had to investigate the cause of the diff; but I also wondered about the missing space. Now, fixing that would have been easy, but I did not want to touch it right now, simply because I am trying to focus on adding changes to HPC-GAP, not modifying the GAP library in the process. So, in order to not forget about it, I logged this issue. But when looking at the code printing that error, I noticed the other things I described in this issue, and also noticed that similar error messages appear in a few other files (when I was grepping for usage,).

Of course similar "issues" exist in many error messages in GAP, and I think those should all also be fixed! But I was not in the process of searching for additional similar errors -- hence I reported only those I was actively aware of. To put it short: An issue report saying "feature X has issue Y" neither means that nothing else has issue Y, nor that X has no other issues :-).

And, lest I forget to mention this: Of course in the end, having "uniform" error messages is not a very important goal in itself, and I'd prefer a non-uniform but helpful error message over a uniform but bad one, or to even none at all! But I think it is still useful to have some consistency here. Nevertheless, if this issue tracker supported setting issue "priorities", I'd have set this one to "priority: low".

To address some specific things you wrot:

• Unfortunately, there is no explicit description of how an error message in the GAP library "should" look like. But as a substitute, one can try to look at all existing errors, and try to imitate them. Of course this imperfect, esp. since those already (as you correctly point out) are already not uniform. But at least one can try to imitate one of the existing styles.
• I do not claim to know how "correct" GAP error messages should look like. So, my report definitely is subjective to a high degree; one of the reasons I put it out there is precisely so that people can comment on it and debate my suggestion -- which you did, and for which I am very grateful!
• You are right, many error message include the word "usage"; the word turns up over 300 times in lib, and many of those seem to be in error messages. But I also note that ~250 of those hits are for "usage: " -- while for "usage,", there are only 19, of which 18 are the ones this issue talks about (the final one is ghom.gi, and should IMHO also be modified). Another 9 messages contain "usage : " (note the extra space), and IMHO those also should be adjusted.
• To clarify, when I say "adjusted", I am personally also happy if those are changed to "usage:". And if for some reason they absolutely must stay on "usage,", I'll also survive (see "priority: low"), though then at least the missing spaces should of course be added.
• Having a trailing comma or newline as a workaround for a general substandard display of error messages in GAP is something I'd really like to avoid -- instead we should strive to improve how we display all error messages. The first step towards that is IMHO to report such issues. So I would be greatly interested in your ideas on how errors could/should be displayed better, ideally in the form of further issues.
• As to function names in error messages: You are of course completely right that many other error messages contain them, too. Again, I did not mean to single out your code, it's just that I happened to write an issue on these particular error message, and so it was logical (to me, at that time) to also point that out. But I should have explained my reasons for why I am wary about adding function names to error messages: it is easy to forget adjusting the messages when function are renamed, and they can contain typos; basically, the same kind of problem all kinds of code or content duplication face. So, ideally, Error and friends should automatically display the function name. However, as I already mentioned in my original report that the backtrace does not display it -- and I also meant to say (but, upon re-reading what I wrote, failed to do so -- oops :-/ totally my bad, sorry!) that until Error resp. the backtracing is improved, the function name should definitely stay in the error message (and perhaps also afterwards, this is something else to be debated). I submitted issue Error backtrace should print names of operations/attributes/... in which the error occurred #1196 on this; though one might want to go further and try to print the name of the function at the start of the error (not sure that's a good idea, though...)
• As to newlines inside error messages: Thing is that most people do not actually use 80 char wide terminals (e.g. I use 98 or 202 wide terminals when using my laptop screen; wider ones when using an external monitor). Ideally, error messages should be readable everywhere. That may sometimes mean there should be a newline (or multiple) in it. But we shouldn't combat bad wrapping with it. If GAP wraps errors badly, we should improve that. So I now filed issue Error() should word-wrap error messages, not break them in the middle of a word #1197 based on your example.

[james-d-mitchell]

Thanks for the clarification @fingolfin, I didn't take your issue personally in any way, I just wanted to understand what was being requested and why. If Issues #1196 and #1197 resolved, this would be really good. In the mean time, I will fix these error messages along the lines you suggest.