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.

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

Conversation

Penumbra69
Copy link

Issue (#3342)

  • Make aliases available to lua
  • Ensure aliases typed in gui/launcher show proper help entry
  • Enhance help entry with aliased command information
  • added a simple set of tests

 - Make aliases available to lua
 - Ensure alieses typed in `gui/launcher` show proper help entry
 - Enhance help entry with aliased command information
 - added a simple set of tests
{
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.

 - Adjusting to take in a table / vector for "addAlias"
 - Adjusting to take in a table / vector for "addAlias"
@lethosor lethosor changed the title Issue 3342 - Make aliases great again Expose Alias API to Lua May 27, 2023

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.

@lethosor
Copy link
Member

Which PR addresses this point?

Enhance help entry with aliased command information

I don't see changes to helpdb in either.


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.

@@ -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.

@Penumbra69
Copy link
Author

Which PR addresses this point?

Enhance help entry with aliased command information

I don't see changes to helpdb in either.

I must have misinterpreted the need to have the alias item show up as part of the helpdb itself, and focused mainly on ensuring that if a user typed an alias, it would show the correct help for them. I will dig into adjusting to make the alias ALSO appear in the helpdb list.

@myk002
Copy link
Member

myk002 commented Jun 8, 2023

I think that would be required, yeah, otherwise it wouldn't show up as an entry in ls and other places.

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