Show value warnings in debug by vadi2 · Pull Request #4678 · Mudlet/Mudlet

vadi2 · 2021-01-25T08:34:22Z

Brief overview of PR changes/additions

Show errors (or warnings actually?) in debug

Motivation for adding to Mudlet

So these messages aren't lost if scripts don't do their error checking.

Don't show to main window because it's not important enough of a message to start spamming with.

Other info (issues closed, discussion etc)

Start addressing #4559.

PR open for review against a few functions, once approved I'll work on the rest.

…-debug

add-deployment-links · 2021-01-25T08:34:46Z

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

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-25T09:34:24Z

Thinking about it, it might be wise to merge this not into development but into #4661 so we don't get massive conflicts.

Kebap · 2021-01-25T15:29:34Z

src/TLuaInterpreter.cpp

 }
 // No documentation available in wiki - internal function
-// See also: verifyBoolean
+// Raises a Lua error in case of an API usage mistake


Good idea, but rename these to type and value errors instead for differentiation

Kebap · 2021-01-25T15:30:15Z

src/TLuaInterpreter.cpp


    if (!mudlet::self()->getAvailableFonts().contains(font, Qt::CaseInsensitive)) {
-        lua_pushnil(L);
-        lua_pushfstring(L, "font '%s' is not available)", font.toUtf8().constData());


nice hard-to-spot bug again eliminated in passing by standardizing (the message ended in ) but should not)

To avoid confusion would it be better to change font to fontName?

That is discussed in #4638

I don't think anyone would actually really be so confused about this that they'd make an error to be honest

Kebap · 2021-01-25T15:36:24Z

Thinking about it, it might be wise to merge this not into development but into #4661 so we don't get massive conflicts.

I hear the thoughts about conflicts, but I tried to keep #4661 to type errors only, trying not to touch value errors at all.
Would rather keep these issues separated not to inflate scope and make reviews even more complicated for others.

Kebap · 2021-01-25T15:53:15Z

src/TLuaInterpreter.cpp

    if (result.first == 2) {
-        lua_pushnil(L);
-        lua_pushfstring(L, "current selection invalid in window \"%s\"", windowName.toUtf8().constData());
+        returnWrongArgumentType(L, __func__, QStringLiteral(R"(current selection invalid in window "%1")").arg(windowName));


Can we agree to just use 'single quotes' inside the string, and do away with R"( syntax?

I don't mind the syntax but I'm okay with changing to single quotes

Some use R"( currently, some escape \" every single quote, some do single quotes, some no quotes at all.
It's just a wild mess and rather incoherent. No obvious direction which style to use for new messages.
I would vote for porting \" and R"( to single quotes, then later add them where no were before.
This way 'argument values' are clearly distinct from parts of the sentence even for casual players.

TBH I am not a fan of the C++ raw string literal form - partly because it broke when used with texts for translations - though that has now been fixed - and I am conditioned to use the escape form - though perhaps I can eventually be deconditioned/deprogrammed away from that. As it is the ' does work better within Lua erorr message, it is just that as a C/C++ coder I am also programmed to think of that delimiter only for single chars so forgot to use that form.

🤯

Kebap · 2021-01-25T22:15:18Z

Watch out as these messages now use %1 syntax whereas type errors use %d and %s instead.

edit: Any idea how to unify?

vadi2 · 2021-01-26T14:49:40Z

Unify - hmm, not without adding overhead I don't think. It's not an issue that bites us in practice.

src/TLuaInterpreter.cpp

SlySven · 2021-01-26T15:58:35Z

src/TLuaInterpreter.cpp

@@ -3179,12 +3122,7 @@ int TLuaInterpreter::saveProfile(lua_State* L)
        return 2;
    } else {
        auto message = QString("Couldn't save %1 to %2 because: %3").arg(host.getName(), std::get<1>(result), std::get<2>(result));


ℹ️ Needs to be switched from a QString to a QStringLiteral to match our coding style/guide IMHO.

Maybe but not in scope really

SlySven · 2021-01-26T16:06:53Z

src/TLuaInterpreter.cpp

-            lua_pushnil(L);
-            lua_pushfstring(L, "setBackgroundColor: bad argument #%d value (red value needs to be between 0-255, got %d!)", s, r);
-            return 2;
+            return warnArgumentValue(L, __func__, QStringLiteral("bad argument #%1 value (red value needs to be between 0-255, got %2!)").arg(s, r));


Since you are now using QString::arg(...) you have got to either use a separate .arg(int).arg(int) form or convert them to text before one of the QString::arg(...) overloads thinks that the second number is a field width or number base or sommat - note that this applies to all cases where there is more than one argument and the second (and possibly higher) arguments are numbers but I am only pointing out this one!

Suggested change

return warnArgumentValue(L, __func__, QStringLiteral("bad argument #%1 value (red value needs to be between 0-255, got %2!)").arg(s, r));

return warnArgumentValue(L, __func__, QStringLiteral("bad argument #%1 value (red value needs to be between 0-255, got %2!)").arg(QString::number(s), QString::number(r)));

Good catch. Need to review!

It's good for mixed argument types but not in general https://github.com/Mudlet/Mudlet/blob/development/.github/CONTRIBUTING.md#use-argarg1-arg2-instead-of-argarg1argarg2

Then again, some of these are like (auto, auto) 😕

Oh whoopsie, will switch to QString::number(x) then instead.
These lines now seem to get rather long and unwieldy.
But I hesitate to split it into 2-3 every single time.

SlySven · 2021-01-26T17:14:56Z

Would it look better if the text began with something like:
LUA WARN: or LUA WARNING:
instead of Lua:
so that it is similar in form to the LUA OK:

Given that you are reporting the function it might be worth checking the format compared to the other ones (the LUA ERROR ones) that also report the function to see if that can be done in a more consistent style...

vadi2 · 2021-01-26T17:34:48Z

You bring up a valid reason, I don't think we should do it - for aesthetic reasons I think an orange Lua is enough and self explanatory. Could even trim Lua OK to just Lua in green.

SlySven · 2021-01-26T17:42:28Z

Well, as I say, there are green LUA OKs and red LUA ERRORs so why can't we have orange LUA WARN(ING)s?

vadi2 · 2021-01-29T15:46:19Z

We also have warnings coming from Host.cpp where they're used as-is, for example:

And then the __func__ doesn't work quite so hot anymore 😅

…-debug

vadi2 · 2021-01-29T18:17:07Z

OK, this looks good to me now, please review.

Kebap · 2021-01-30T00:04:24Z

src/Host.cpp

            pC->resize(width, height);
            pC->move(x, y);
-            return {false, QStringLiteral("miniconsole \"%1\" exists already. moving/resizing \"%1\".").arg(name)};
+            return {false, QStringLiteral("miniconsole \"%1\" exists already: moving/resizing \"%1\".").arg(name)};


I don't understand this change. The : makes this confusing. Why did the . not suffice? There are more changes like this I don't understand.

It's proper grammar to use : to extend the thought instead of starting a separate sentence - looks better on the eye.

What other ones didn't you follow?

"exists already" or "already exists" - I can't put my finger on it but the second sounds better - especially as an introduction to what follows after the :

I agree, latter sounds better.

No idea how : connects the 2 sentences. I would expect information on where or how the name exists already.
I would connect the sentences with "therefore" or leave them separated or drop the second completely.

SlySven · 2021-01-30T20:08:56Z

#4678 (comment)

I am not so sure that the double use of : works so well there - what is on the left side of one is an introduction or something that is given more detail on the right side - but how does that work when there is more that one? See https://en.wikipedia.org/wiki/Colon_(punctuation)

vadi2 · 2021-01-30T20:13:10Z

Where are there cases with more than one?

SlySven · 2021-01-30T20:20:00Z

LUA: createLabelMainWindow: label "svo.riftlabel" exists already.

One ':' after "LUA" and another after "createLabelMainWindow"!

vadi2 · 2021-01-30T20:23:01Z

That doesn't seem like an issue to me. Why add all sorts of different symbols just to be different

Kebap

Don't like the ':' added: The rest of the PR is great

(See how : doesn't make sense above. )

src/Host.cpp

Kebap · 2021-01-31T11:52:05Z

src/Host.cpp

            pC->resize(width, height);
            pC->move(x, y);
-            return {false, QStringLiteral("miniconsole \"%1\" exists already. moving/resizing \"%1\".").arg(name)};
+            return {false, QStringLiteral("miniconsole \"%1\" exists already: moving/resizing \"%1\".").arg(name)};


No idea how : connects the 2 sentences. I would expect information on where or how the name exists already.
I would connect the sentences with "therefore" or leave them separated or drop the second completely.

src/Host.cpp

vadi2 · 2021-01-31T12:52:13Z

Feedback applied

This was introduced in the recent "Show value warnings in debug" PR Mudlet#4678. I believe I have found all of use of `%d` (or `%s`) instead of `%1` or higher numbers in this file. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

) This was introduced in the recent "Show value warnings in debug" PR #4678. I believe I have found all of use of `%d` (or `%s`) instead of `%1` or higher numbers in this file. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

…dlet#4765) This was introduced in the recent "Show value warnings in debug" PR Mudlet#4678. I believe I have found all of use of `%d` (or `%s`) instead of `%1` or higher numbers in this file. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2 added 2 commits January 25, 2021 09:28

Add returnWrongArgumentType() to show errors in debug as well

6d3cb5f

Merge remote-tracking branch 'origin/development' into show-errors-in…

717543e

…-debug

vadi2 requested a review from a team as a code owner January 25, 2021 08:34

vadi2 requested review from a team January 25, 2021 08:34

Kebap mentioned this pull request Jan 25, 2021

Standardize + announce nil errors #4681

Closed

Kebap reviewed Jan 25, 2021

View reviewed changes

Kebap added 7 commits January 25, 2021 16:54

+2

e004261

make it return as well

6090ef6

make it return as well

12ae4de

rename to warnArgumentValue

cd6722c

+20

f2c6428

+5

90a49c7

typos

5a856e4

Kebap added 2 commits January 25, 2021 23:22

.constData()

560ab37

+10

72ea4a2

SlySven reviewed Jan 26, 2021

View reviewed changes

src/TLuaInterpreter.cpp Outdated Show resolved Hide resolved

SlySven reviewed Jan 26, 2021

View reviewed changes

arg(a,b) to arg(a).arg(b)

0453871

Kebap added 4 commits January 28, 2021 22:11

Remove "bad argument #n value"

def0b26

Remove more "bad argument #n value"

f8c13a0

even more warnArgumentValue()

e5c6661

fixes

c64b7e9

Kebap mentioned this pull request Jan 29, 2021

don't speak unless given text, ignore > and < #4706

Merged

vadi2 added 6 commits January 29, 2021 16:51

Remove periods from window creation messages

43f00e0

Merge remote-tracking branch 'origin/development' into show-errors-in…

2dab80f

…-debug

Fix warning message for createLabel

c29d04f

Improve clarity for openWindow()

cf52823

Replace double quotes with single quotes

645b1e4

Quotes and ending period fixes

7af7733

Kebap reviewed Jan 30, 2021

View reviewed changes

Kebap approved these changes Jan 31, 2021

View reviewed changes

Review feedback

76eac74

vadi2 merged commit 3929ed9 into Mudlet:development Feb 1, 2021

vadi2 deleted the show-errors-in-debug branch February 1, 2021 08:07

vadi2 mentioned this pull request Feb 4, 2021

Map Info contributors - adjustable map info #4718

Merged

SlySven mentioned this pull request Feb 8, 2021

BugFix: correct lua string vs QString replacement argument mixups #4765

Merged

vadi2 mentioned this pull request Nov 28, 2021

Mention nil+errors in debug console #4559

Closed

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

Show value warnings in debug console (Mudlet#4678)

b96488a

	return warnArgumentValue(L, __func__, QStringLiteral("bad argument #%1 value (red value needs to be between 0-255, got %2!)").arg(s, r));
	return warnArgumentValue(L, __func__, QStringLiteral("bad argument #%1 value (red value needs to be between 0-255, got %2!)").arg(QString::number(s), QString::number(r)));

Uh oh!

Conversation

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

Uh oh!

Test versions

Uh oh!

vadi2 commented Jan 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kebap Jan 25, 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!

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 25, 2021

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

Uh oh!

Uh oh!

vadi2 commented Jan 26, 2021

Uh oh!

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 Jan 26, 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 commented Jan 26, 2021

Uh oh!

vadi2 commented Jan 26, 2021

Uh oh!

SlySven commented Jan 26, 2021

Uh oh!

vadi2 commented Jan 29, 2021

Uh oh!

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

Uh oh!

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!

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

edited

Loading

Kebap Jan 25, 2021 •

edited

Loading

Kebap commented Jan 25, 2021 •

edited

Loading

Kebap Jan 26, 2021 •

edited

Loading

vadi2 commented Jan 29, 2021 •

edited

Loading