Adding some functions to work with Lua tables.

[Danielkonge]

I will add documentation later, but here is a first draft of some functions for working with tables in lua. This gives some of what was asked for in #4328.

[Danielkonge]

I think this is ready for feedback now.

[wez]

Thanks for this!
Sorry that it's taken a while to get around to reviewing it!

[bew]

For to_string what about using this 'inspect' implementation?
(either rewrite it or embed it in wezterm.inspect)

https://github.com/kikito/inspect.lua
Iirc this the inspect impl used in some projects like neovim.

[Danielkonge]

For to_string what about using this 'inspect' implementation? (either rewrite it or embed it in wezterm.inspect)

https://github.com/kikito/inspect.lua Iirc this the inspect impl used in some projects like neovim.

I guess it depends what people use to_string for. If you are mainly interested in inspecting the items of a table (and don't care much about the look/formatting of the table), then I think the current to_string_fallback is already fine. If you want a more custom formatting, then we do need to do something extra at least (be that using inspect, the current solution or something else).

[wez]

Since wezterm has custom embedded types that have their own string representation, it is important that what we provide for this string representation is:

• Aware of internal wezterm types (and thus cannot be implemented purely in lua, ruling out that inspect module)
• Ideally consistent with how wezterm prints strings in the debug overlay, and performs implicit string conversion in wezterm.log_xxx and its print implementation
• Not a parallel and slightly different implementation from ValuePrinter, which is already more code than I'd like to be maintaining; I'd hate to have to double the amount of that kind of code.

I think that means that we should stick to the ValuePrinter for this purpose.
That isn't actually a table-specific function, and could be exposed as wezterm.to_string.

[Danielkonge]

Since wezterm has custom embedded types that have their own string representation, it is important that what we provide for this string representation is:

• Aware of internal wezterm types (and thus cannot be implemented purely in lua, ruling out that inspect module)
• Ideally consistent with how wezterm prints strings in the debug overlay, and performs implicit string conversion in wezterm.log_xxx and its print implementation
• Not a parallel and slightly different implementation from ValuePrinter, which is already more code than I'd like to be maintaining; I'd hate to have to double the amount of that kind of code.

I think that means that we should stick to the ValuePrinter for this purpose. That isn't actually a table-specific function, and could be exposed as wezterm.to_string.

I haven't yet looked at the ValuePrinter code, but I can try to look into exposing it via wezterm.to_string. Do you think that this PR shouldn't include to_string and to_string_fallback then?

You said the ValuePrinter is fine with printing recursive tables right? Have you tried to use it to print something like _G (from Lua)? That is where I currently use the fallback. (Can you even do this now, or is it not available from Lua at all at this point?)

Update:
I tried making to_string use the ValuePrinter. Here is how it looks:

Compared to to_string_fallback:

Note: Here to_string_fallback better understands the format:

Compared to to_string with ValuePrinter:

[Danielkonge]

For the last commit, it is only meant to be temporary for now, if you want to try out to_string using the ValuePrinter here. It should probably be moved to wezterm.to_string instead of wezterm.table.to_string later.

[wez]

re: utf8.charpattern, 83fbba5 handles that better in ValuePrinter.

[Danielkonge]

re: utf8.charpattern, 83fbba5 handles that better in ValuePrinter.

Another thing to consider for the ValuePrinter is if we want it to list the type (and/or address) of a recursive element similar to how to_string_fallback points to _G as the first entry of the table, where ValuePrinter skips it (because we are printing _G here, so it is ignored since it has been visited by the ValuePrinter.

We could just keep the ValuePrinter as is, and then keep to_string_fallback in this commit for when someone really needs to know all the entries of a table, but I am not sure if it would be better this way?

[aarondill]

I'd prefer entering the 'branch' of the table, as it can be much more useful in common problems. for a few examples from my own wezterm config: "Is config.colors.foreground set?" "is config.colors.copy_mode_active_highlight_bg.Color set?" "is config.window_padding.left set?"

Perhaps it would even be possible to return the value at the key path if it is set? I don't know how well rust interacts with multiple return types (can you use Result with a tuple?).

local has, color = has_key(config, 'copy_mode_active_highlight_bg', 'Color')
if has then
-- do something with color
else
-- color is nil, do something else
end

Since the rust code already has to traverse the entire table path, this would only add one .get call (since we now need to get the last value, instead of just checking presence)

[Danielkonge]

I'd prefer entering the 'branch' of the table, as it can be much more useful in common problems. for a few examples from my own wezterm config: "Is config.colors.foreground set?" "is config.colors.copy_mode_active_highlight_bg.Color set?" "is config.window_padding.left set?"

Perhaps it would even be possible to return the value at the key path if it is set? I don't know how well rust interacts with multiple return types (can you use Result with a tuple?).

local has, color = has_key(config, 'copy_mode_active_highlight_bg', 'Color')
if has then
-- do something with color
else
-- color is nil, do something else
end

Since the rust code already has to traverse the entire table path, this would only add one .get call (since we now need to get the last value, instead of just checking presence)

We could add a functionality like that too, but wouldn't this make more sense to put into its own function called something like get? Though code like what you wrote would of course traverse the path to the key twice then, but that shouldn't be very expensive.

[Danielkonge]

I added optional extra keys to has_key now, but it took longer than expected because of some annoyance with mlua's .get() and .as_table(), so I didn't get to the docs stuff yet. I will continue working on this tomorrow.

[Danielkonge]

The docs are mostly fixed now, and I have cleaned up a few other things. @wez: In extend and deep_extend do you think it makes sense to just construct a Table without a specified size and move the second loop up to the first loop? Then we can simplify the code a bit.

[wez]

Should these equality operations use whatever == maps to in rust, or should these be calling lua_value_eq?

Probably not always. See my comment below about closures. I don't really want to go down a rabbit hole on that in this PR, so my suggestion would be: just keep this with the shallow level search for this PR and then you can revisit in a follow up if you're feeling motivated!

[Danielkonge]

I will just remove the behavior parameter for now then, and only support a shallow search in this PR.

As to ==, I think this should be equivalent to mlua's .eq(), but I can change it to that to make it clearer what is going on.

[Danielkonge]

Or do you mean .equals()? I think that is a custom method whereas .eq() is just derived.

[wez]

TBH, I'm not sure about this Deep value search operation. I can't think of a single time I've needed something like this in 25+ years. The closest I've come is a DFS on a graph, but in that situation I've needed to apply a closure rather than look for a full value.

[Danielkonge]

I can just remove it to simplify the code for now. I thought it might be useful to see if certain values were precent in one's config, but in that case you would usually know what key it should be set for anyways.

[Danielkonge]

Actually looking at the updated code, I don't really think it adds much extra. Though I do agree that it will probably only be rarely used. Do you prefer to just remove it?

[aarondill]

I can't quite read the source for "impl_lua_conversion_dynamic!" well enough to know if it already is, but is it possible to make the behavior enums case insensitive? if not then don't worry about it, but since we're using behavior strings and these won't be accessible as enums from lua, i'd prefer (personally) to use lowercase strings.

case insensitive would provide the best of both worlds, as well as ensuring that users don't have to remember whether each method needs capitalized or lowercase strings.

[Danielkonge]

I think I have addressed most of the feedback now, but I still need to edit the following:

• has_value (code + docs): Do we want to just remove the behavior and only do the "Shallow" case?
• clone (code + docs): Do we want to move this to wezterm.clone? In that case, how should it clone UserData? (Also, in which crate should we put this function?)
• Should we rename "Keep" and "Force" to something like "KeepFirst" and "TakeLast"?

[Danielkonge]

There are probably a few minor things left to edit in the code, but I think it is close enough that I can start to write some tests. @wez is there any other place in the code that does testing of similar Lua function implementations that I can base my tests on? Otherwise I will have to spend a little time and read the mlua docs more carefully.

Update: I have added a couple of example tests now. Please let me know if they look okay or if you would want a different kind of test setup? Then I can continue writing more tests.

Also, looking back at the code I made an observation:
For functions like e.g. extend, we can take in a LuaMultiValue to get a list of tables instead of an array with the tables in it, if we put the behavior string first and make it non-optional. Would you prefer that setup? (This is the way neovim has chosen for some similar functions.)

[Danielkonge]

@wez I kind of forgot about this. Could you give some feedback on the last two comments? Then I can go through and clean this up during this weekend (and maybe some of next week).