Skip to content
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

Merged
merged 1 commit into from
Apr 10, 2019

Conversation

ChrisJefferson
Copy link
Contributor

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.

@ChrisJefferson ChrisJefferson added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Apr 7, 2019
@codecov
Copy link

codecov bot commented Apr 7, 2019

Codecov Report

Merging #3394 into master will decrease coverage by 12.48%.
The diff coverage is n/a.

@@             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
Impacted Files Coverage Δ
src/plist.c 80.05% <ø> (ø)
lib/meatauto.gi 5.99% <0%> (-89.52%) ⬇️
lib/nilpquot.gi 11.11% <0%> (-88.89%) ⬇️
lib/grpcompl.gi 4.54% <0%> (-81.07%) ⬇️
lib/reesmat.gi 27.14% <0%> (-72.04%) ⬇️
lib/randiso2.gi 2.57% <0%> (-67.65%) ⬇️
lib/filter.gi 33.33% <0%> (-66.67%) ⬇️
lib/semitran.gi 31.28% <0%> (-66.57%) ⬇️
lib/tcsemi.gi 5.76% <0%> (-65.39%) ⬇️
lib/memusage.gi 13.51% <0%> (-64.87%) ⬇️
... and 376 more

@coveralls
Copy link

coveralls commented Apr 7, 2019

Coverage Status

Coverage remained the same at 85.207% when pulling e83e9f8 on ChrisJefferson:nicer_tnum_names into 7ea1f43 on gap-system:master.

Copy link
Member

@wilfwilson wilfwilson left a 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.

src/plist.c Outdated Show resolved Hide resolved
src/plist.c Outdated Show resolved Hide resolved
@@ -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)
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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);

@ChrisJefferson
Copy link
Contributor Author

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.

src/plist.c Show resolved Hide resolved
src/plist.c Show resolved Hide resolved
@fingolfin
Copy link
Member

Seems not all test files were updated, there are diffs in tst/testinstall/list.tst and tst/testinstall/listindex

@fingolfin fingolfin merged commit bd34ff4 into gap-system:master Apr 10, 2019
@ChrisJefferson ChrisJefferson deleted the nicer_tnum_names branch May 8, 2019 16:53
@DominikBernhardt DominikBernhardt added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 21, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants