kernel: fix Issue 1434 #1435
kernel: fix Issue 1434 #1435
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1435 +/- ##
==========================================
+ Coverage 63.39% 63.39% +<.01%
==========================================
Files 1010 1010
Lines 337899 337917 +18
Branches 13550 13554 +4
==========================================
+ Hits 214217 214230 +13
- Misses 120692 120695 +3
- Partials 2990 2992 +2
|
src/trans.c
Outdated
| ErrorQuit("the second argument must consist of positive integers " | ||
| "but found %s", | ||
| (Int)TNAM_OBJ(val), 0L); | ||
| } else if (INT_INTOBJ(val) > LEN_LIST(t)) { |
There was a problem hiding this comment.
Could use n here instead of LEN_LIST(t) (very minor comment)
There was a problem hiding this comment.
Right, will do
james-d-mitchell
force-pushed
the
fix-issue-1434
branch
from
e922e5f to
9aacc86
Compare
src/trans.c
Outdated
|
|
||
| if (!IS_LIST(l)) { | ||
| ErrorQuit("the first argument must be a list but found %s", |
There was a problem hiding this comment.
In most (?) other kernel error mssages, we would say " must be a list (not a %s)".
I don't mind the first part so much, but "(not a %s)" is used quite consistently everywhere, and "but found %s" is not.
Of course this applies to all other error messages, too.
There was a problem hiding this comment.
Right, I used "found %s" because the first thing I tried it on printed "(not a integer)" which I found difficult :(
There was a problem hiding this comment.
Sure, that's something annoying we show in lots of place right now, though. If it bothers you, we could start an effort to correctly print a/an by providing a function that returns the TNAM string, together with a or an correctly prefixed.
Or else, a PR that changes (not a %s) everywhere.
But I am not so happy about mixing this...
There was a problem hiding this comment.
Agreed, I'll change this back to (not a %s)
There was a problem hiding this comment.
And think about making a TNAM_OBJ_WITH_ARTICLE
james-d-mitchell
force-pushed
the
fix-issue-1434
branch
from
9aacc86 to
7191eea
Compare
|
@fingolfin I've made the changes now. |
|
But forgot to update the tests... |
There were no checks on the arguments of the function IsInjectiveTransList, this commit introduces such checks, and associated tests.
james-d-mitchell
force-pushed
the
fix-issue-1434
branch
from
7191eea to
cf4d6a8
Compare
|
All done, and ready to go. |
There were no checks on the arguments of the function
IsInjectiveTransList,this PR introduces such checks, and associated tests. The documentation of this function was also incorrect (the arguments were the wrong way around), and this is also fixed in this PR.