rephrase "wrong argument type" errors by Kebap · Pull Request #4599 · Mudlet/Mudlet

Kebap · 2021-01-08T11:20:49Z

Brief overview of PR changes/additions

Bring all TLuaInterpreter functions forward to current standards.

Motivation for adding to Mudlet

Never just show "wrong argument type" again without informing which argument, which type expected, etc.

Other info (issues closed, discussion etc)

Another large step for #2091 mostly wielding #4593 where possible

add-deployment-links · 2021-01-08T11:20:51Z

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

linux: https://make.mudlet.org/snapshots/f3264e/Mudlet-4.10.1-testing-pr4599-7dd6f4ed-linux-x64.AppImage.tar
osx: (download pending, check back soon!)
windows: https://make.mudlet.org/snapshots/6849ea/Mudlet-4.10.1-testing-pr4599-7dd6f4ed-windows.zip

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

vadi2 · 2021-01-08T13:53:57Z

Kebap · 2021-01-08T14:11:57Z

Yep that was quite a text reducing spree 😁
It's nice how often a frightening complicated if-return spaghetti can be reduced to a flat and easy to digest 5-line function.
I actually took the time to go through all 80 functions last mentioned in the attached issue and fixed almost all of them.

Only a few doubts remain:

not sure how insertLink handles its arguments or verification or indeed how to intervene there at all
setPopup seems to handle arguments differently than wiki documentation implies, e.g. accepts string as argument Profile keyboard navigation #2 but should be table

Noted but not in scope:

refactor all functions' existing standardized error messages to use new "verified" syntax
add another WINDOW_NAME macro for optional arguments e.g. like in insertPopup or moveCursor
add getVerifiedTable for table arguments e.g. like insertPopup
add getVerifiedDouble for numbers unlike int and float e.g. like in moveWindow etc. or setTriggerStayOpen
add getVerifiedNumberOrString for arguments that can be either of those types
make sure all functions check all their arguments for correct type at all before processing like clearUserWindow or spawn
rename c++ functions hideUserWindow to hideWindow and showUserWindow to showWindow to reflect their lua function names
decide if setWindowWrap should treat window name as optional argument (status quo) or expected value (wiki docs as well as status quo e.g. for setWindowWrapIndent)
rename c++ variables to reflect their lua meaning, like variable "text" in enableTrigger etc. which means the trigger's name
maybe refactor WINDOW_NAME from macro into function, as SlySven helped me do in [Refactor] Streamline error message creation #4593 yesterday, RE Next round of cleanups #4167 (comment)

vadi2 · 2021-01-08T17:29:31Z

not sure how insertLink handles its arguments or verification or indeed how to intervene there at all

I suspect it doesn't do it too well, need to investigate

setPopup

Need to update manual docs in this case

SlySven

There are other places where this code can be refactored in, e.g.:

addRoom(...)
setDoor(...)

👍 Ah, you are only targetting the "wrong argument" cases at the moment - leaving scope for further refactoring...!

SlySven · 2021-01-08T22:23:38Z

src/TLuaInterpreter.cpp

-    }
-    int luaSleepMsec = lua_tointeger(L, 1);
-
+    int luaSleepMsec = getVerifiedInt(L, __func__, 1, "sleep time in msec");


👍 I'd forgotten about __func__!

SlySven · 2021-01-08T22:40:43Z

src/TLuaInterpreter.cpp

-    if (!dirType) {
-        lua_pushstring(L, "setExitStub: Need a dir number as 2nd argument");
+    int dir = dirToNumber(L, 2);
+    if (!dir) {


🤔 For an invalid string (not matching those defined in dirToNumber(...)) or a invalid number (not in range 1 to 12) that function will also return a 0 but that is not a type error but a run-time value error so should not be aborting the calling Lua script - only an invalid argument type should be doing that - therefore there should be a
if (!(lua_isnumber(L, 2) || lua_isnumber(L, 2))) {
test before the dirToNumber(...)call in this area IMHO - and that should trigger the error here.

😊 Ooops - meant to say:
if (!(lua_isnumber(L, 2) || lua_isstring(L, 2))) {

SlySven · 2021-01-08T22:47:21Z

src/TLuaInterpreter.cpp

    }
-    pR->setExitStub(dirType, status);
+    pR->setExitStub(dir, status);
    return 0;


Given all the other error handling changes I guess it would not be unreasonable to actually return a true if we get here to indicate success!

SlySven · 2021-01-08T22:47:25Z

src/TLuaInterpreter.cpp

-    if (dirType > 12 || dirType < 1) {
-        lua_pushstring(L, "setExitStub: dirType must be between 1 and 12");
+    if (dir > 12 || dir < 1) {
+        lua_pushstring(L, "setExitStub: direction must be between 1 and 12");


The range of dir has already been clamped to 0 to 12 by dirToNumber(...) and that first value is the error case that represents either an invalid direction number OR an unrecognised as an (Engineering-English) exit string. So this text needs revising. Also as those are run-time value errors they should return a nil+error message and not abort the calling Lua script...

... the message should, of course not contain setExitStub: but actually indicate the number or string that was supplied by the caller.

If you are not going to address this other problem then we'll need a new issue along the lines of:
"Some lua functions that take a (normal) exit as a direction number or a string return confusing error messages for invalid exit strings"
i.e. give the exit direction as "downwards" and the function will abort with a message
"xxxx: direction must be between 1 and 12" - even though a string rather than a number was given.

SlySven · 2021-01-08T22:53:34Z

src/TLuaInterpreter.cpp


    if (!lua_isnumber(L, 2)) {
-        lua_pushstring(L, "resizeWindow: wrong argument type");
+        lua_pushfstring(L, "resizeWindow: bad argument #2 type (x as number expected, got %s!)", luaL_typename(L, 2));


width not x

SlySven · 2021-01-09T00:19:55Z

src/TLuaInterpreter.cpp

+    float height = getVerifiedFloat(L, __func__, 7, "height");
+    float zoom = getVerifiedFloat(L, __func__, 8, "zoom");
+    bool showOnTop = getVerifiedBoolean(L, __func__, 9, "zoom");



💭 There is now an option to NOT scale the image - haven't tried playing with it to see how it acts in conjunction with the scaling - but it might be desirable to add the later added feature to the Lua API.

SlySven · 2021-01-09T00:24:08Z

src/TLuaInterpreter.cpp

-    bool gridMode = lua_toboolean(L, 2);
-
+    int area = getVerifiedInt(L, __func__, 1, "areaID");
+    bool gridMode = getVerifiedBoolean(L, __func__, 2, "true/false");


grid mode rather than true/false - like the other cases it is implicit that true means grid mode "on".

SlySven · 2021-01-09T00:25:18Z

src/TLuaInterpreter.cpp

-    }
-    QString sendText = lua_tostring(L, 1);
-
+    QString sendText = getVerifiedString(L, __func__, 1, "sendText");


HTML not sendText here.

SlySven · 2021-01-09T00:26:13Z

src/TLuaInterpreter.cpp


    if (!lua_isstring(L, 2)) {
-        lua_pushstring(L, "setLabelStyleSheet: wrong argument type");
+        lua_pushfstring(L, "setLabelStyleSheet: bad argument #2 type (markup as string expected, got %s!)", luaL_typename(L, 2));


Maybe style sheet rather than markup ?

SlySven · 2021-01-09T00:27:08Z

src/TLuaInterpreter.cpp

-            return lua_error(L);
-        }
-        QString tidiedWhat = QString(lua_tostring(L, 1)).toLower().trimmed();
+        QString tidiedWhat = getVerifiedString(L, __func__, 1, "style", true).toLower().trimmed();


format rather than style perhaps?

Kebap · 2021-01-09T00:53:15Z

Isn't it funny how much more obvious naming and logic become once the clutter is out the way? 😀

I took most name suggestions from wiki documentation (like the one letter names, and in fact the wide variety of names for color components in different functions) and do not plan aligning those during this PR right here. Unless of course you found some more mix ups and copy paste errors, which I gladly take up.

Also most other logic remained unchanged, so all 💭 should move to their own issue for future reference.
(There was nothing stopping the suggested changes before, only now they became more obvious maybe?)

The functions calling dirToNumber() I actually found quite difficult to touch because they mix type and value error handling, in an external function, with an argument that can be multi type as well. I hope I did not destroy any existing logic here, merely trying to add an understandable message to the (messy) status quo.

Kebap · 2021-01-09T10:53:22Z

@SlySven FYI I commented your bunch of suggestions with thumb up and down which indicate NOT if I support the suggestion in general, as there were actually quite a few I liked, like better names for variables etc. BUT if they led to changes included in this PR scope after all or no changes done.

SlySven · 2021-01-09T12:16:12Z

The thing is that you are introducing names for arguments to the lua functions - in describing that they are wrong - so it makes sense so that the terms that are used makes sense. So for the elements of a RGB colour specification, for instance "red (outer) color component as number expected, got xxx" reads better than "color1Red as number expected, got xxx".

Kebap · 2021-01-09T12:35:08Z

for instance "red (outer) color component as number expected, got xxx" reads better than "color1Red as number expected, got xxx".

And both read better than the current wrong argument type.

The thing is that you are introducing names for arguments to the lua functions

Wrong, the wiki (remember what's the central source of documentation #1150 😏) introduced these names long ago:

Kebap · 2021-01-09T17:39:59Z

If you are not going to address this other problem then we'll need a new issue along the lines of:
"Some lua functions that take a (normal) exit as a direction number or a string return confusing error messages for invalid exit strings"
i.e. give the exit direction as "downwards" and the function will abort with a message
"xxxx: direction must be between 1 and 12" - even though a string rather than a number was given.

Yes please do open a new issue to untangle all dirToNumber usages. It's easy, just a click on the green button at top, and collect ideas and code snippets there. In fact, would be better than spreading all the ideas over multiple comment threads in this pr as well as duplicated again in the new issue you're hopefully already starting to create.

Why a down-thumb? Rather than giving even a "wrong argument type" error for a wrong 2nd argument it is currently being silently ignored if it is not a number - yet that is something that your fancy new functions could handle.

The functions came in #4593 and I'd be glad to see them used more. However this PR here merely uses them to remove all "wrong argument type" messages. While doing that I noted a bunch additional things and listed them above as out-of-scope. Thumbs relate to scope as explained above. Feel free to tackle those currently ignored errors or any of the other points really.

#4599 (comment)

SlySven · 2021-01-10T00:08:40Z

Yes please do open a new issue ...

Done.

Kebap · 2021-01-15T23:43:31Z

setPopup

Need to update manual docs in this case

No, that seems like a copy/paste error, compare insertPopup() where that string is used as well, but in setPopup() it is never used at all. How should we update the docs then?

rephrase all errors like "wrong argument type"

3c67332

Kebap added the enhancement label Jan 8, 2021

Kebap added this to the 5.0 beginner-friendly milestone Jan 8, 2021

Kebap self-assigned this Jan 8, 2021

vadi2 approved these changes Jan 8, 2021

View reviewed changes

Kebap changed the title ~~[Refactor] rephrase remaining "wrong argument type" error messages~~ rephrase remaining "wrong argument type" error messages Jan 8, 2021

Kebap added 4 commits January 8, 2021 12:26

+10

081ac6f

+6

46fcf1f

+6

68231cb

+18

c182839

Kebap added 2 commits January 8, 2021 14:01

+13

d471535

+1

8bcadbd

+3 undocumented functions

dd66e37

Kebap marked this pull request as ready for review January 8, 2021 15:40

Kebap requested a review from a team as a code owner January 8, 2021 15:40

Kebap requested review from a team January 8, 2021 15:40

Kebap changed the title ~~rephrase remaining "wrong argument type" error messages~~ rephrase "wrong argument type" error messages Jan 8, 2021

Kebap changed the title ~~rephrase "wrong argument type" error messages~~ rephrase "wrong argument type" errors Jan 8, 2021

Kebap added 5 commits January 8, 2021 15:43

typo

a7760be

Bugfix

4b83a54

Copy paste error

f6a5837

fix setExit

ebc8306

fix setGridMode

b82c936

vadi2 approved these changes Jan 8, 2021

View reviewed changes

SlySven reviewed Jan 9, 2021

View reviewed changes

vadi2 approved these changes Jan 9, 2021

View reviewed changes

fixes as per peer review

7dd6f4e

This was referenced Jan 9, 2021

Refactor: move labels from TMap to each TArea instance. #4604

Merged

Some lua functions that take a (normal) exit as a direction number or a string return confusing error messages for invalid exit strings #4605

Closed

vadi2 mentioned this pull request Jan 14, 2021

Modernize exists() function #4618

Closed

Kebap merged commit b735529 into development Jan 14, 2021

Kebap deleted the rephrase_wrong_argument_type branch January 14, 2021 10:27

This was referenced Jan 14, 2021

[refactor] switch common error checks to getVerifiedX #4620

Closed

repair how insertLink handles arguments #4621

Closed

add getVerifiedDouble #4623

Closed

Rename a few internal functions #4624

Merged

more understandable parameter names everywhere #4638

Open

Kebap mentioned this pull request Jan 25, 2021

Standardize error messages in TLuaInterpreter.cpp #2091

Closed

Chris7 pushed a commit to Chris7/Mudlet that referenced this pull request Jan 2, 2022

rephrase "wrong argument type" errors (Mudlet#4599)

fc4e09f

Uh oh!

Conversation

Kebap commented Jan 8, 2021

Brief overview of PR changes/additions

Motivation for adding to Mudlet

Other info (issues closed, discussion etc)

Uh oh!

add-deployment-links bot commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test versions

Uh oh!

vadi2 commented Jan 8, 2021

Uh oh!

Kebap commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vadi2 commented Jan 8, 2021

Uh oh!

SlySven left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kebap commented Jan 9, 2021

Uh oh!

Kebap commented Jan 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SlySven commented Jan 9, 2021

Uh oh!

Kebap commented Jan 9, 2021

Uh oh!

Kebap commented Jan 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SlySven commented Jan 10, 2021

Uh oh!

Kebap commented Jan 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

add-deployment-links bot commented Jan 8, 2021 •

edited

Loading

Kebap commented Jan 8, 2021 •

edited

Loading

Kebap commented Jan 9, 2021 •

edited

Loading

Kebap commented Jan 9, 2021 •

edited

Loading