Feature/early eval for parameterized options

[ppcamp]

Description of changes:

Add support to early eval an option item.

# same as used to be
cat <file=$(ls)>

# same as used to be
cat <file=|__|_$(ls)_|>

# same as used to be
cat <file=|_opt1_|_opt2_|>

# ADDED
cat <file=|_$(ls)_|>

In this case (last scenario), it'll execute ls using cmd/runner/Run function (which keeps record of folder and shell to use), before create the gui, therefore, allowing user to just select one of the output elements.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[RamiAwar]

Thanks for the contribution! Will take a look at it soon and play around with it.

[RamiAwar]

Interesting approach you went with - I like that you didn't break existing behavior to do this.

I'm still thinking about this, reviewed part of it so far, will try to finish it today.

At a high level, I think lazy-evaluated parameters is a pretty cool concept. I think it belongs in Pet, can't see why not. It does add some complexity and risks bugs but if it's an isolated feature then should be ok.

I don't LOVE the fact that we're using the multi-default-value for this - maybe it's better to create a special representation here, but we risk creating too many special formats making pet hard to use - but also it's not too bad given that there's no reason for anyone to use multi-default-value repr for a single value.

On the other hand, we are blocking people from ever using multiple lazy-evaluated default values (ex. having 2 commands as separate choices). But that seems super unlikely to me - hard time thinking of a realistic usecase for that.

Idk, let this thought simmer maybe we come up with some other idea worth considering.

[RamiAwar]

lgtm generally. left some small comments.

One more thing: please document this well in the README so people know how to do this.

Optionally, if you're able to, consider also documenting multi-default value usage in the 'pet new' command description. I believe that's still lacking.

[RamiAwar]

This function feels a bit strange - the fact that it accepts a string starting and ending with $() feels weird to me usage wise.

It might read a little nicer if we break it down one more step. Here we can relax the strictness of the check inside EvaluateDynamicOptions (works for any string now, if no dynamic options found inside it it'll just error out). We will have to regex match inside the evaluate now though, but

input := "pgcli -h $(get_available_hosts) -p 5432"
if hasDynamicOptions(input) {
    dynamicOptions, err := EvaluateDynamicOptions(input)
    // do stuff
}

[ppcamp]

@RamiAwar just updated the PR with your suggestions Pls review it again.

[RamiAwar]

Better, but still is a bit off. I'd like hasDynamicOptions to also support dynamic options that are in the middle of the string, not just strings that start with "$(" and end with ")".

This makes it easier to use, harder to misuse.

[ppcamp]

got it. Will work on that.

---

Just added the support to use in the middle of the string (allowing to have a prefix/suffix)

However, as described in the godoc, I'm still evaluating just the outermost command,
so if the user plan to have several commands nested like $(command $(with $(nested $(output) ) ), it'll took the whole string and send it to /bin/sh, so the shell will be responsible to evaluate with and nested, and therefore, if you don't have support to it, like cmd, than you cannot do nested evaluations.

Is it ok with this approach?

[RamiAwar]

That makes sense!

[RamiAwar]

isn't it simpler to do without the slice? adding then removing the 2 is more confusing. It's mainly due to the indx variable re-assignment that this is happening.

Something like:

start, end := indx[0] + 2, index[1] - 1
prefix := rawcmd[:start]
suffix := rawcmd[end:]
nestedCmd := rawcmd[start:end]