[Refactor] Streamline error message creation by Kebap · Pull Request #4593 · Mudlet/Mudlet

Kebap · 2021-01-07T15:51:33Z

Brief overview of PR changes/additions

Use dedicated functions to verify lua argument types given by users

Motivation for adding to Mudlet

Less repetition, more standardisation, easier readability and writability

Other info (issues closed, discussion etc)

Helpful tool to tackle a probably large part of #2091

Current draft as proof of concept can be tested with createMapLabel()
https://wiki.mudlet.org/w/Manual:Lua_Functions#createMapLabel

labelID = createMapLabel(areaID, text, posx, posy, posz, fgRed, fgGreen, fgBlue, bgRed, bgGreen, bgBlue, zoom, fontSize, showOnTop, noScaling)

edit: All arguments of this function's have been ported to new style messages.

~~The following arguments have been ported to new style messages for testing of many possible style combinations:~~

~~areaID - integer~~
~~text - string~~
~~posX - float~~
~~zoom - optional float~~
~~showOnTop - optional boolean~~

Try giving wrong argument types to any of these ~~(and the non-listed unchanged ones)~~ and see what happens.

…style

add-deployment-links · 2021-01-07T15:51:57Z

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

linux: https://make.mudlet.org/snapshots/d87000/Mudlet-4.10.1-testing-pr4593-5e54c951-linux-x64.AppImage.tar
osx: (download pending, check back soon!)
windows: https://make.mudlet.org/snapshots/b827fb/Mudlet-4.10.1-testing-pr4593-5e54c951-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.

Kebap · 2021-01-07T19:11:59Z

May need to refactor into macro, because the function should return the object, but may also return int upon failure.

SlySven · 2021-01-07T20:27:59Z

src/TLuaInterpreter.cpp

+// The "isOptional" parameter is optional, and will default to not-optional parameters! :)
+//
+// See also: verifyString, verifyInt, verifyFloat, announceWrongArgumentType
+bool TLuaInterpreter::verifyBoolean(lua_State* L, const int pos, const QString& functionName, const QString& publicName, const bool isOptional)


As the strings are squirted straight into a lua_pushfstring(...) you do not need to dress them up as QStrings - const char* is perfectly fine in this particular circumstances...

That might help with all those errors further down! 😜

Which errors are you referring? Is this wrong or merely inconvenient?
I for one know QString better than char* so I used what I knew already.

cannot pass object of non-trivial type 'const QString' through variadic function; call will abort at runtime [-Wnon-pod-varargs]

You cannot use a QString as a parameter to go into a %s in a lua_pushfstring(...) call, you need to convert it back to a const char* with a QString::toUft8() followed by a QByteArray::constData() call - so you might just as well avoid a round trip from const char* ==> QString ==> const char* ...

SlySven · 2021-01-07T20:31:03Z

src/TLuaInterpreter.cpp

+{
+    if (!lua_isstring(L, pos)) {
+        announceWrongArgumentType(L, pos, functionName, publicName, "string", isOptional);
+        return lua_error(L);


Guess you just have to use:

lua_error(L); Q_UNREACHABLE(); return {};

here.

Thanks for the suggestion!

…/Mudlet into streamline_error_messages

Kebap · 2021-01-07T22:18:05Z

Try giving wrong argument types to any of these (and the non-listed unchanged ones) and see what happens.

This seems to work now:

vadi2

Yep, it looks good! I'd rename the verify suffix to something else, since the primary purpose is not to verify, but to retrieve the data value. Verification is the secondary purpose.

Will you be removing the old code / doing up the rest of the function?

Kebap · 2021-01-08T05:41:13Z

I actually found verify a short and exact description of what happened, but am open to suggestions. Maybe getVerifiedString instead?

I will complete createMapLabel and remove comments as part of this PR. The other functions are part of the linked issue instead.

vadi2 · 2021-01-08T05:49:39Z

To me verify just means that, verify. I was confused as to where the actual data retrieval comes in then. getVerifiedString sounds good 👍

Kebap · 2021-01-08T08:40:26Z

Motivation for adding to Mudlet

Less repetition, more standardisation, easier readability and writability

I like this, less clutter, less noise, more to the point information:

The original had double size still, cut for brevity

vadi2

With this improvement the API would be like "static argument, dynamic argument, static argument, dynamic argument" - making it a bit of a pain to copy/paste around - might be worthwhile to change it to "static argument, static argument, dynamic argument, dynamic argument"

src/TLuaInterpreter.cpp

vadi2 · 2021-01-08T08:48:24Z

#4593 (comment) yeah, great improvement for sure!

Kebap · 2021-01-08T08:53:33Z

With this improvement the API would be like "static argument, dynamic argument, static argument, dynamic argument" - making it a bit of a pain to copy/paste around - might be worthwhile to change it to "static argument, static argument, dynamic argument, dynamic argument"

I actually had it like that before:
posZ = getVerifiedFloat(L, "createMapLabel", 5, "posZ");
But then changed it to this:
posZ = getVerifiedFloat(L, 5, "createMapLabel", "posZ");
Because we already use a lot of these:
posZ = lua_tonumber(L, 5);
And they seem easier to migrate this way.

You argument holds merit, too. Not sure which is worse.. 🤔

edit: Let's try it out again!

Co-authored-by: Vadim Peretokin <vperetokin@gmail.com>

vadi2 · 2021-01-08T09:23:30Z

Very nice solution to something we've been stuck on for a long while!

Co-authored-by: Vadim Peretokin <vperetokin@gmail.com>

Kebap added 9 commits January 7, 2021 15:08

Add functions for streamlined argument verification

e3665ac

Differentiate int and float verification

18da8df

Add createMapLabel proof of concept

b2d7d51

Switch function definition: pos before functionName like lua_*(L, n) …

d1283e7

…style

Make notOptional an optional parameter

679bef7

Untangle double negations

edbab69

Enlarge PoC to include all styles at least once

62957cc

Rename verifyBool to verifyBoolean for consistency

f8010a4

Update code documentation to actual code

ba153d8

Kebap and others added 5 commits January 7, 2021 16:04

Typo

987d55d

remove extra qualification ‘TLuaInterpreter::’

fe1fcbd

switch function signature, optionals last

41ffd97

try static function definitions

edd333e

Fix up function signatures

eaad178

SlySven reviewed Jan 7, 2021

View reviewed changes

Kebap added 4 commits January 7, 2021 21:08

seperate return from lua_error()

bcf60cb

Merge branch 'streamline_error_messages' of https://github.com/Mudlet…

113e236

…/Mudlet into streamline_error_messages

replace QStrings with const char*

12ed6f9

return empty QString (but don't)

8bf2cd7

Kebap marked this pull request as ready for review January 7, 2021 22:18

Kebap requested a review from a team as a code owner January 7, 2021 22:18

Kebap requested review from a team January 7, 2021 22:18

typo

db853e0

vadi2 reviewed Jan 8, 2021

View reviewed changes

Kebap added 4 commits January 8, 2021 08:20

rename verifyX functions to getVerifiedX

84cb385

finish migration of createMapLabel

91b4dab

typo

61f647c

align documentation to code

df3f675

vadi2 reviewed Jan 8, 2021

View reviewed changes

src/TLuaInterpreter.cpp Outdated Show resolved Hide resolved

src/TLuaInterpreter.cpp Outdated Show resolved Hide resolved

src/TLuaInterpreter.cpp Outdated Show resolved Hide resolved

src/TLuaInterpreter.cpp Outdated Show resolved Hide resolved

Kebap and others added 2 commits January 8, 2021 09:57

Switch to using __func__ instead

714b7b4

Co-authored-by: Vadim Peretokin <vperetokin@gmail.com>

Switch function definition: pos after functionName

6352fb0

vadi2 approved these changes Jan 8, 2021

View reviewed changes

Switch function calls as well

5e54c95

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

Kebap merged commit 48f9727 into development Jan 8, 2021

Kebap deleted the streamline_error_messages branch January 8, 2021 10:36

This was referenced Jan 8, 2021

rephrase "wrong argument type" errors #4599

Merged

[refactor] switch common error checks to getVerifiedX #4620

Closed

refactor WINDOW_NAME macros into functions #4622

Closed

Kebap mentioned this pull request Jan 15, 2021

Add get verified double #4639

Merged

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

[Refactor] Streamline error message creation (Mudlet#4593)

df2cbb2

Co-authored-by: Vadim Peretokin <vperetokin@gmail.com>

Uh oh!

Conversation

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

Uh oh!

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 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test versions

Uh oh!

Kebap commented Jan 7, 2021

Uh oh!

SlySven Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

SlySven Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

Kebap Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

Kebap Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

Kebap commented Jan 7, 2021

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

Kebap commented Jan 8, 2021

Uh oh!

vadi2 commented Jan 8, 2021

Uh oh!

Kebap commented Jan 8, 2021

Motivation for adding to Mudlet

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Kebap commented Jan 7, 2021 •

edited

Loading

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

edited

Loading

SlySven Jan 7, 2021 •

edited

Loading

Kebap commented Jan 8, 2021 •

edited

Loading