feat(layout): Support environment variables in cwd (#2288)

[shahamran]

Fixes #2288.

• Add shellexpand as dependency. I hope it's fine, it only has dirs as a dependency.
• Expand environment variable in kdl parser's parse_cwd().
  • On failure, fall back to previous behavior.

It's my first time creating a PR here. Tell me if I missed anything! 😃

[imsnif]

Hey, I'm happy to see this! On the whole this looks good. I left one minor comment in the code, but there are some behaviours that I think we need to look more deeply into.

1. shellexpand mentions that they can also handle tildes (~) at the beginning of paths. So that we can do something like ~/my/folder that will be expanded to /home/aram/my/folder. Could we also support those while we're here?
2. We need to make sure this also works and does the right thing with cwd composition in its various permutations (also to be sure that we do the right thing after addressing point 1... maybe only invoke shellexpand for the whole string?)
3. Be sure we support cwd for bare panes (eg. pane cwd="my/folder"), command panes (eg. pane command="ls" cwd="my/folder") and edit pans (eg. pane edit="my_file.rs" cwd="my/folder"). All of these should behave properly with env variables and tildes

Thanks again for this! Looking forward to seeing this merged soon.

[shahamran]

Hi. Thanks for the quick reply!
I actually thought about adding tilde expansion, but thought it's a big feature which will less likely be approved.
I'll add it (replace shellexpand::env() with full()) and more tests. My plan is to replace this explicit, fragile test with snapshot + manual observation. What do you think?

[shahamran]

Hi @imsnif,
Do you want me to make additional changes?

[imsnif]

Hey @shahamran - sorry for taking so long to get to this!

Good progress! I did some manual tests and found that there are some places in which we're still not picking this up. I think this needs to work in the layouts wherever we have a path, otherwise it might be confusing. I didn't test everything, but just a few examples:

layout {
    pane cwd="~/code" // works!
    pane edit="~/backup/foo.txt" // does not work for me
    pane command="~/backup/executable" // does not work for me
    pane command="ls" { // works!
        cwd "~/backup"
    }
}

Would you like to take a look at these and maybe do another sweep of the layout file to find out if there might be other places we'll need to add this functionality to?

[shahamran]

I see. In this case, I think the 2 most reasonable options are:

1. Modify kdl_get_string_property_or_child_value_with_error to perform shellexpand internally.
2. Have a general parse_path() method in KdlLayoutParser and use it when parsing cwd, edit, command (and risk missing places where it's also applicable).

Regarding 1, why is this a macro? couldn't it be a regular function that takes a &KdlNode and &str?
I searched for usages and didn't encounter anything that justifies a macro. What am I missing?

[imsnif]

Regarding 1, why is this a macro? couldn't it be a regular function that takes a &KdlNode and &str? I searched for usages and didn't encounter anything that justifies a macro. What am I missing?

I dunno, it works :)

Do whatever you think is best, let me know when you'd like another review. I'll try to get to it ASAP but thank you for your patience!

[shahamran]

Hi, sorry for the long delay.
I added handling and tests for the command and edit contexts.
I think the more robust solution will be to do it in the kdl-parser module, but I didn't manage to make a small change there, I ended up changing the entire file, which was too much IMO.
Hope this smaller commit catches all use cases.
Let me know what you think!

[imsnif]

Hey, great work! Thanks for making all the changes. This looks to be in great shape - I left some minor comments in the code - I think once they are addressed we can merge. Looking forward to seeing this in main.

[imsnif]

I think this will set the environment variable globally for all the tests, no? I'm a little wary of such things. Ideally we can find a solution that does this locally only for one test, and if not maybe we can unset these after the assert?

[shahamran]

that's true, and this is why I used weird names for the vars I set (besides $HOME).

current version handles restoring the previous env after parsing.

[imsnif]

This is great, but I personally find it a little hard to understand at a glance. Which I think is important when we're considering tests. How about we change it to something like (pseudocode, if you have a better idea then by all means!)

LayoutTesterWithEnvironment::new()
    .set_env_variables(/* ... */)
    .parse_layout(stringified_layout)
    .assert(/* .. */)
    .assert(/* .. */)

[imsnif]

What do you think about asserting the cwd in the parsed structures (iirc they are a TildPaneLayout in this case) directly instead of parsing and stringifying? I think it'll be more accurate and clear, but maybe it's too much work?

[shahamran]

I think the snapshot test I added handles this request indirectly 😃

[shahamran]

Hi, thanks for the feedback :)

I agree the test helper thing was ugly and fragile. I merged the tests to a single snapshot test, and went over the generated file.

I also handled adding / removing env vars in a more correct way.

Let me know if this looks good to you, and feel free to request as many changes as needed!
I appreciate the feedback.

[shahamran]

@imsnif ping :)

[imsnif]

Hey @shahamran - thank you for your patience. It's not always easy for me to get to everything that needs attention in the project in a timely manner, so I appreciate your understanding.

Great work on the changes, I'm happy to go ahead and merge this. Thanks for your diligent work and perseverance!

[shahamran]

Thanks a lot @imsnif, for the guidance and feedback.

While working on this, I noticed a place that I think can be improved, and I'm not sure opening an issue is the right place for it, so I'm asking here:
Will you be open for a PR that refactors / changes the kdl parsing module from macro-based to function-based? I think it'll help with readability, allow code-completion in these functions and improve compile times.

[imsnif]

Hey @shahamran - while I very much appreciate the offer, I think if we consider refactoring this area of the code it would be best to re-write it in the new KQL (I believe it is called) - the KDL query language.

But I'm afraid even that is not in my focus right now. This project is still pre 1.0, which means most of the changes that happen are on a larger scope (eg. adding a web client). There will be lots of time for refactoring and improving the developer experience once things stabilize a bit. More info about what we're focusing on here.

If you're interested in helping out in those areas, you can hop by discord and ask for specific guidance on issues in our development channel.

Thanks again for your contribution!

[MurtadhaInit]

Thanks for the great work!

I just wanted to ask whether this was also supposed to fix the issue of env variables not accepted as values for options within the config file or not.

The reason I'm asking is that something like scrollback_editor doesn't accept a value of $EDITOR, or default_shell not accepting $SHELL (which actually contradicts the docs).

Sorry if that's unrelated or if this is not the right place to raise this.

[shahamran]

Hi @MurtadhaInit. Can you open an issue, and I'll try to look into it?

Please include steps to reproduce the problem. Thanks!

[MurtadhaInit]

@shahamran Hi, I've opened an issue 2574
Let me know if there are any further details I can provide.