Skip to content

Modernize exists() function#4618

Closed
vadi2 wants to merge 1 commit intoMudlet:developmentfrom
vadi2:modernize-exists
Closed

Modernize exists() function#4618
vadi2 wants to merge 1 commit intoMudlet:developmentfrom
vadi2:modernize-exists

Conversation

@vadi2
Copy link
Copy Markdown
Member

@vadi2 vadi2 commented Jan 14, 2021

Brief overview of PR changes/additions

Modernize exists() function

Motivation for adding to Mudlet

Better error messages

Other info (issues closed, discussion etc)

@vadi2 vadi2 requested a review from a team as a code owner January 14, 2021 01:07
@vadi2 vadi2 requested review from a team January 14, 2021 01:07
@add-deployment-links
Copy link
Copy Markdown

add-deployment-links bot commented Jan 14, 2021

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.

Copy link
Copy Markdown
Contributor

@Kebap Kebap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this part of #4599 needs to be prioritized over the rest of the bunch, but since I am the author of that, clearly I approve this, too

@vadi2
Copy link
Copy Markdown
Member Author

vadi2 commented Jan 14, 2021

#4599 is good to go with me!

@vadi2
Copy link
Copy Markdown
Member Author

vadi2 commented Jan 14, 2021

@vadi2 vadi2 closed this Jan 14, 2021
@vadi2 vadi2 deleted the modernize-exists branch January 14, 2021 01:23
@Kebap
Copy link
Copy Markdown
Contributor

Kebap commented Jan 14, 2021

Actually your type changes are not covered there, so this still has merit. 😉

@vadi2 vadi2 restored the modernize-exists branch January 14, 2021 01:28
@vadi2 vadi2 reopened this Jan 14, 2021
@vadi2
Copy link
Copy Markdown
Member Author

vadi2 commented Jan 14, 2021

Want to merge your PR first? Otherwise I'll give you a conflict 😢

type = type.toLower();
if (type == "timer") {
if (type == QLatin1String("timer")) {
cnt += host.getTimerUnit()->mLookupTable.count(name);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of += is deceptive here - as only ONE of the specified types will be checked it is probably clearer to only use = in these assignments...

} else if (type == "script") {
} else if (type == QLatin1String("script")) {
cnt += host.getScriptUnit()->findScriptId(name).size();
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we not be producing a nil + error message about a non-valid type being provided? And if we do THAT then it makes more sense for each type to be refactored to the form of:

-    } else if (type == QLatin1String("button")) {
-        cnt += host.getActionUnit()->findActionsByName(name).size();
-    } else if (type == "script") {
+   if (type == QLatin1String("button")) {
+        lua_pushnumber(L, host.getActionUnit()->findActionsByName(name).size());
+        return 1
+   }
-    } else if (type == "script") {
+   if (type == "script") {


Host& host = getHostFromLua(L);
int cnt = 0;
type = type.toLower();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be done as part of (new) line 7962 could it not?

Copy link
Copy Markdown
Member

@SlySven SlySven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 but I think we can also handle the run-time error of an invalid (or even empty) second argument...

@Kebap
Copy link
Copy Markdown
Contributor

Kebap commented Jan 15, 2021

Want to merge your PR first? Otherwise I'll give you a conflict 😢

Thanks, done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants