-
Notifications
You must be signed in to change notification settings - Fork 160
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
Give the plist TNUMS more human-readable names #3394
Conversation
45e1e77
to
f115c7d
Compare
Codecov Report
@@ Coverage Diff @@
## master #3394 +/- ##
===========================================
- Coverage 73.3% 60.81% -12.49%
===========================================
Files 534 640 +106
Lines 265929 313299 +47370
===========================================
- Hits 194950 190547 -4403
- Misses 70979 122752 +51773
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I support the idea of making the names more readable.
On the whole, I'm happy with what you've done, and I don't think we should worry unduly about the new names being long.
But since we're doing this change, I think we should think carefully about the choices.
In particular, I think that we should be consistent about the ordering of the adjectives. For example, you've got new names homogeneous strictly-sorted plain list
and non-strictly-sorted homogeneous plain list
; in one, the homogeneity comes first, in other the sortedness. I don't mind which you choose, but I think consistency amongst the names will, in my opinion, make these types as easy for people to understand as possible.
Actually, I do have one preference about ordered: for each type X with name Y, I would prefer the immutable version of X to have name "immutable Y", i.e. the immutable adjective would always be the first one. Somehow it is more important to me. Of course, just because it's my preference doesn't mean you have to change to accommodate it, but I'd be interested to hear what anyone else feels about this.
I think non-strictly-sorted is fine, I think using set would be weird, you'd either have things like "homogeneous set plain list", and having both set and list there is strange, or to avoid that you'd have "homogeneous plain set" which I think doesn't fit well with the rest of the names.
@@ -60,7 +60,7 @@ Error, Field getter: <data> must be a small integer (not a ffe) | |||
gap> bf.setters[1](1, (1,2)); | |||
Error, Field Setter: <val> must be a small integer (not a permutation (small)) | |||
gap> bf.setters[1]([],1); | |||
Error, Field Setter: <data> must be a small integer (not a list (plain,empty)) | |||
Error, Field Setter: <data> must be a small integer (not a empty plain list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here is the unfortunate side effect of your nice changes. "not a empty..." (or "not a immutable..." or whatever), when the correct English would be "not an empty..." (or "not an immutable..."). As an English speaker, this is something that mildly upsets me, but I can live with this. Do we try to make sure that indefinite articles (a/an) are correct in GAP? I'd guess not. I can imagine that it would be a pain to make this always work properly, so I'm not going to suggest that we should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably wouldn't be that hard to fix :) We could have an investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, making this right was (for me) one of the driving motivations (yes, seriously :-) for introducing those Require*
helpers (changed not a integer
to not an integer
and then later not the integer %d
). So, I'd really want to improve this, at least when using the Require
macros. One trick option here would be to modify RequireArgumentEx
. Like this:
diff --git a/src/error.c b/src/error.c
index ff156757a..f7b071f35 100644
--- a/src/error.c
+++ b/src/error.c
@@ -590,8 +590,19 @@ Obj RequireArgumentEx(const char * funcname,
else if (op == Fail)
strlcat(msgbuf, " (not the value 'fail')", sizeof(msgbuf));
else {
- strlcat(msgbuf, " (not a %s)", sizeof(msgbuf));
- arg1 = (Int)TNAM_OBJ(op);
+ const char * tnam = TNAM_OBJ(op);
+ // check for starting vowel
+ // TODO: move this into a separate helper function?
+ // TODO: improve this, and e.g. use "a" before "un"; "an hour"; ...
+ // see also <https://github.com/rossmeissl/indefinite_article/blob/master/lib/indefinite_article.rb>
+ // alternatively, could enhance the TNAM records to (optionally) allow indicating whether 'an' or 'a'
+ // should be used....
+ if (strchr("aeiou", tnam[0]))
+ strlcat(msgbuf, " (not an", sizeof(msgbuf));
+ else
+ strlcat(msgbuf, " (not a", sizeof(msgbuf));
+ strlcat(msgbuf, tnam, sizeof(msgbuf));
+ strlcat(msgbuf, ")", sizeof(msgbuf));
}
ErrorMayQuit(msgbuf, arg1, arg2);
f115c7d
to
e122e56
Compare
I meant to give a consistent ordering to the words, but obviously slipped up. I've moved all the mutables to the front, and I've ordered the other things by their order in the TNUM name, as it seems like an easy way to get a consistent ordering. |
Seems not all test files were updated, there are diffs in |
e122e56
to
f1a5262
Compare
f1a5262
to
e83e9f8
Compare
We are increasingly printing out the TNUMs of types, and I had someone at GAP days ask me what "list (plain, nhom)" meant. A reasonable question!
This replaces the names with longer human-readable names. I'm happy for tweaks to the names (I find non-strictly-sorted long. I thought about writing "set" in that case, or non-strictly sorted?). This does make them much longer.
I notice we seem to use both homogeneous and homogenous, I went with the first, as it is (I believe) correct, and we use it much more.