BugFix: try and make connectExitStub(...) work as per the API#5395
Conversation
This PR should enable connectExitStub(...) to work as close to the existing
published API as possible:
* connectExitStub((integer) fromRoomID, (integer)toRoomID,
(integer or string) direction)
where direction is the initial(s) or full, lower-case, un-hypenated ENGLISH
word for one of the 12 normal exit directions - this will make a two way
exit between the given direction of the fromRoomID room to the toRoomID
room AND the corresponding reverse direction exit from the toRoomID room.
The rooms need not be the same Area but they must BOTH have stub exits in
the required direction.
* `connectExitStub((integer) fromRoomID, (integer) toRoomID)` - this will
make a two way exit between the fromRoomID room to the toRoomID room AND
the corresponding reverse direction exit from the toRoomID room. The rooms
need not be the same Area but the fromRoomID must have only ONE stub exit
with an opposite (reverse) direction one in the toRoomID one - either room
can have other stub exits. Should there be more than one pair of stub exits
a nil + error message will be produced listing the choices for the
direction that can be passed to the three argument function call to make
the exits wanted.
* `connectExitStub((integer) fromRoomID, (integer or string) dirction)`
- this will make a two way exit between the stub exit in the fromRoomID
room to the NEAREST other room IN THE SAME AREA which has a stub exit in
the reverse direction and which lies in the correct relative position
(except for the `in`/`out` directions where this is not relevant). Should
the direction be given as an integer in the range 1 to 12 this will be
rejected (via a nil + error message) because it is ambiguous then whether
the number represents a direction or a toRoomID. Potentially unlike the
prior code, this version properly detects whether a string or number is
supplied as the direction argument.
Also:
* to allow the reporting of the direction as a number and a string in error
messages the `(QString) TRoom::dirCodeToString(const int)` method has been
made `static` so that it can be used in the `TLuaInterpreter` class.
* as indirectly mentioned above
`(int) TLuaInterpreter::dirToNumber(lua_State*, int)` has been revised to
check for a string or integer argument being examined - the prior code
may not work as anticipated because it used `lua_isxxxx(...)` functions
which can coerce the value they are dealing with (a number can be coerced
into a string) - which messes with the logic.
* in places in the revised methods the integer constant values have been
replaced with the `DIR_XXXXX` values defined in the `TRoom` class header
file.
* to simplify (!) the coding the three different forms of the Lua API are
implemented in three separate `(QString) TMap::connectExitStubByXxxx(...)
methods which collectively replace the original
`(void) TMap::connectExitStub(...)` one. They are responsible for
generating most of the Lua API error messages for this function and they
indicate success by returning an empty string.
This should close Mudlet#2386.
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
Hey there! Thanks for helping Mudlet improve. 🌟 Test versionsYou can directly test the changes here:
No need to install anything - just unzip and run. |
So that the two argument `connectExitStub(...)` works as good as it can with the second argument being a number (between 1 and 12). Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
A return was in the wrong place so a value set was never used as the function would exit before it was accessed. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
You can only skip putting in an `else` if it was actually proceeded with a `return` - in the place I had messed up it wasn't. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
This PR needs to be updated with latest |
…orkAsPerTheAPI Needed to pull in development branch changes that include a Lua testing framework. Signed-off by: Stephen Lyons <slysven@virginmedia.com>
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
@Xekon able to test this? |
vadi2
left a comment
There was a problem hiding this comment.
It looks good to me, thanks for fixing the bug with it.
Do you have energy to create either C++ or Lua-based tests to ensure this complicated function keeps working in the future as intended?
| lua_pushstring(L, "connectExitStub: RoomId doesn't exist"); | ||
| return lua_error(L); | ||
| if (lua_gettop(L) < 2) { | ||
| lua_pushfstring(L, "connectExitStub: missing argument #2 (toID as number or direction as number or string expected)"); |
There was a problem hiding this comment.
| lua_pushfstring(L, "connectExitStub: missing argument #2 (toID as number or direction as number or string expected)"); | |
| lua_pushfstring(L, "connectExitStub: missing argument #2 (toID as number or direction number/string expected)"); |
@Kebap your thoughts on the wording?
There was a problem hiding this comment.
That's not how we do things around here anymore. We now have functions to phrase the error messages in order to unify the user experience and ease developer workflow.
However in case of these overloaded functions, which can accept either number or string in a same position, the functions are not yet smart enough. So we need to still manually craft the error message to mimic the automatic formula.
You can test any function and leave an obligatory argument to see the error message reads like this:
tempLineTrigger: bad argument #2 type (how many lines to match for as number expected, got no value!)
So for your case we land round about here:
| lua_pushfstring(L, "connectExitStub: missing argument #2 (toID as number or direction as number or string expected)"); | |
| lua_pushfstring(L, "connectExitStub: bad argument #2 type (toID as number, or direction as number/string expected, got no value!)"); |
I got no opinion on whether "or" or slash is better to inform both number and string are possible. I would not drop "as" to seperate argument name and type as that's a pretty established pattern.
There was a problem hiding this comment.
TBH I hadn't looked too closely into the situation where one gets a no value as a/an (non-existent) argument type! Like Kebap says, I did have to manually construct the error message because I couldn't get the required message out of the functions because the getVerifiedXxxx(...) ones are only constructed for a single type - and they try to coerce the provided argument to that type - whereas this one needed to do the exact opposite (i.e. only treat it as a number if it is one and a string likewise). In this case saying that the argument is missing is simpler to read though I agree that it doesn't follow how our messages have tended to go - perhaps we might want to look at our handling of missing required arguments more generally?
This one can be revisited I am sure - but I think we benefit more from the immediate improvement in functionality of this PR as it is then we might lose from stalling as we argue over this detail.
| } | ||
|
|
||
| QString TRoom::dirCodeToString(const int dirCode) const | ||
| /*static*/ QString TRoom::dirCodeToString(const int dirCode) |
There was a problem hiding this comment.
I only picked up on marking with this comment such methods in this manner recently but I think it does help as a reminder when one is doing a global search on the use of the method's name! 🤓
Revise: add a suggestion to the user on how to proceed on a message Signed-off by: Stephen Lyons <slysven@virginmedia.com> Co-authored-by: Vadim Peretokin <vperetokin@gmail.com>
Not really, I understand that we do now have a Lua framework available but I'm not sure I have the mental bandwidth currently to make use of it. I'm just pleased that I have been able to fix a 2.5 year old bug.... |
This PR should enable connectExitStub(...) to work as close to the existing published API as possible:
connectExitStub((integer) fromRoomID, (integer)toRoomID, (integer or string) direction)wheredirectionis the initial(s) or full, lower-case, un-hypenated ENGLISH word for one of the 12 normal exit directions - this will make a two way exit between the given direction of thefromRoomIDroom to thetoRoomIDroom AND the corresponding reverse direction exit from thetoRoomIDroom. The rooms need not be the same Area but they must BOTH have stub exits in the required direction.connectExitStub((integer) fromRoomID, (integer) toRoomID)- this will make a two way exit between thefromRoomIDroom to thetoRoomIDroom AND the corresponding reverse direction exit from thetoRoomIDroom. The rooms need not be the same Area but thefromRoomIDmust have only ONE stub exit with an opposite (reverse) direction one in thetoRoomIDone - either room can have other stub exits. Should there be more than one pair of stub exits anil+ error message will be produced listing the choices for the direction that can be passed to the three argument function call to makethe exits wanted.
connectExitStub((integer) fromRoomID, (integer or string) dirction)- this will make a two way exit between the stub exit in thefromRoomIDroom to the NEAREST other room IN THE SAME AREA which has a stub exit in the reverse direction and which lies in the correct relative position (except for thein/outdirections where this is not relevant). Should the direction be given as an integer in the range 1 to 12 this will be rejected (via anil+ error message)becauseif it is ambiguousthenwhether the number represents a direction or a to room ID {depending on what stub exits the fromRoomID has and whether there is a room with the number that is between 1 an d12}. Potentially unlike the prior code, this version properly detects whether a string or number is supplied as the direction argument.Also:
(QString) TRoom::dirCodeToString(const int)method has been madestaticso that it can be used in theTLuaInterpreterclass.(int) TLuaInterpreter::dirToNumber(lua_State*, int)has been revised to check for a string or integer argument being examined - the prior code may not work as anticipated because it usedlua_isxxxx(...)functions which can coerce the value they are dealing with (a number can be coerced into a string) - which messes with the logic.DIR_XXXXXvalues defined in theTRoomclass header file.(QString) TMap::connectExitStubByXxxx(...)methods which collectively replace the original(void) TMap::connectExitStub(...)one. They are responsible for generating most of the Lua API error messages for this function and they indicate success by returning an empty string.This should close #2386.
Signed-off-by: Stephen Lyons slysven@virginmedia.com
Release post highlight
connectExitStub(...)should now work better in the two argument cases {and return meaningful error messages in all cases!}Edited: to reflect revision to actively check what the second argument as a number might be when there are only 2 arguments.