Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose Alias API to Lua #3423

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Expose Alias API to Lua #3423

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cac2bcd
Issue #3342: https://github.com/DFHack/dfhack/issues/3342
Penumbra69 May 26, 2023
8d18b1c
Issue #3342: https://github.com/DFHack/dfhack/issues/3342
Penumbra69 May 26, 2023
27b6df8
Issue #3342: https://github.com/DFHack/dfhack/issues/3342
Penumbra69 May 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions library/LuaApi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3577,6 +3577,68 @@ static int internal_md5file(lua_State *L)
}
}

/**************************
* Wrappers for Alias API *
**************************/

static int internal_getAliasCommand(lua_State *L)
{
const char *name = luaL_checkstring(L, 1);
std::string ret = Core::getInstance().GetAliasCommand(name);
if (!ret.empty())
lua_pushstring(L, ret.c_str());
else
lua_pushnil(L);
return 1;
}

static int internal_listAliases(lua_State *L)
{
auto aliases = Core::getInstance().ListAliases();
Copy link
Member

Choose a reason for hiding this comment

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

If we add a Lua::Push override for std::vector, this function could be reduced to Lua::Push(L, Core::getInstance().ListAliases()); return 1;

Copy link
Author

Choose a reason for hiding this comment

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

Nice - will look at this.


lua_newtable(L);
for (const auto& p : aliases)
{
Lua::Push(L, p.first.c_str()); // alias
Lua::PushVector(L, p.second);
lua_settable(L, -3);
}
return 1;
}

static int internal_isAlias(lua_State *L)
{
const char *name = luaL_checkstring(L, 1);
lua_pushboolean(L, Core::getInstance().IsAlias(name));
return 1;
}

static int internal_addAlias(lua_State *L)
{
const char *new_alias = luaL_checkstring(L, 1);
std::vector<std::string> alias;
split_string(&alias, new_alias, " ");
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to have this function take a list of arguments instead; otherwise there is no way to set up certain aliases, e.g. with spaces in arguments.

It looks like there is a Lua::GetVector() that could work here. It's a more specific function than its name would apply - it reads strings starting at the stack position you give (1) and pushes them all into a vector<string>, but that looks like it should work for our purposes here.

Copy link
Author

Choose a reason for hiding this comment

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

I'll adjust.

Copy link
Author

Choose a reason for hiding this comment

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

This change is complete, tests have been updated.

Copy link
Member

Choose a reason for hiding this comment

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

I was mistaken - on second look, it looks like GetVector takes a table and converts it to a vector, but your code is expecting that.

Have you tested passing a non-table to make sure the code doesn't crash? An error message is fine (and expected).

Copy link
Author

Choose a reason for hiding this comment

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

I have tested that, and it does crash. I'll take a look and find a way to ensure stability.

Copy link
Member

Choose a reason for hiding this comment

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

luaL_checktype(L, 1, LUA_TTABLE) before the GetVector call would probably work.


if (!alias.empty())
{
std::string name = alias[0];
Copy link
Member

Choose a reason for hiding this comment

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

I think we should match the AddAlias() signature instead by taking two arguments: name (a string) and arguments (a list of arguments). It looks like you're retrieving the alias name from the first element of the list here, which doesn't match the C++ API.

Copy link
Author

Choose a reason for hiding this comment

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

I'll adjust.

alias.erase(alias.begin());
lua_pushboolean(L, Core::getInstance().AddAlias(name, alias));
} else {
lua_pushboolean(L, false);
}

return 1;
}

static int internal_removeAlias(lua_State *L)
{
const char *rem_alias = luaL_checkstring(L, 1);

lua_pushboolean(L, Core::getInstance().RemoveAlias(rem_alias));
return 1;
}

static const luaL_Reg dfhack_internal_funcs[] = {
{ "getPE", internal_getPE },
{ "getMD5", internal_getmd5 },
Expand Down Expand Up @@ -3605,6 +3667,13 @@ static const luaL_Reg dfhack_internal_funcs[] = {
{ "getCommandDescription", internal_getCommandDescription },
{ "threadid", internal_threadid },
{ "md5File", internal_md5file },

{ "getAliasCommand", internal_getAliasCommand },
{ "isAlias", internal_isAlias },
{ "listAliases", internal_listAliases },
{ "addAlias", internal_addAlias },
{ "removeAlias", internal_removeAlias },

{ NULL, NULL }
};

Expand Down
11 changes: 11 additions & 0 deletions test/library/alias.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
-- Simple tests to ensure the alias functions work as expected
function test.aliases()
expect.eq(false, dfhack.internal.isAlias("foo"))
Copy link
Member

Choose a reason for hiding this comment

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

this can actually be flaky since it depends on the current active environment. these function tests allow you to specify a setup/teardown wrapper where you can save the current aliases, clear the alias list for the test, and restore the original aliases afterwards. See the orders plugin tests for an example.

expect.eq(true, dfhack.internal.addAlias("foo help"))
expect.eq(true, dfhack.internal.isAlias("foo"))
expect.eq("help", dfhack.internal.getAliasCommand("foo"))
expect.eq("this is not an alias", dfhack.internal.getAliasCommand("this is not an alias"))
expect.eq("help", dfhack.internal.listAliases()["foo"][1])
expect.eq(true, dfhack.internal.removeAlias("foo"))
expect.eq(false, dfhack.internal.isAlias("foo"))
end