knqyf263 / pet

Simple command-line snippet manager
MIT License
4.58k stars 230 forks source link

Added multiple parameter support via two character separators #290

Closed RamiAwar closed 8 months ago

RamiAwar commented 9 months ago

Issue #, if available: #192, #203, #239, #250, #247

Description of changes:

✨ This introduces support for multiple default values in parameters ✨

Based this off @hitzhangjie's and @morriswinkler 's PRs! Thanks for laying all of the groundwork 😄

The proposed solution would work as follows: <param=|_first value_||_second value here_||_third value can be = anything too _|>

The characters were chosen to minimize conflicts with shell languages, and mirroring was used to reduce that chance even further.

On a technical level, we still match the general parameters in the same way. But before creating the parameter layout for filling the parameter values, we split the value into several values if multiple are present. If not, we keep the same logic as before. This ensures backwards compatibility.

multiple-default-values

RamiAwar commented 9 months ago

I still would like to refactor the code a little, it's a bit unreadable now and hard to navigate. I just hacked this together quickly to get it working and wanted to share it asap.

I also am not a big fan of the switching of parameters as is - it is still lacking visibility. I want to see what other options I have (as in a kind of drop down with highlighting of some sort).

If that turns out to be hard (no idea still, I'm a gocui noob 😬 ) then we can just merge this as is. I want it in by the end of this week so it's included in Sunday's release. This way if people want to use this they already can!

@morriswinkler @hitzhangjie @jaroslawhartman Let me know what you think!

jaroslawhartman commented 8 months ago

Hi @RamiAwar

I also am not a big fan of the switching of parameters as is - it is still lacking visibility. I want to see what other options I have (as in a kind of drop down with highlighting of some sort).

Perhaps easiest would be to add an optional clue when there are multiple values:

image

(sample code: https://github.com/jaroslawhartman/pet/blob/94ff848e4cd364cca28021ae4f9cddea28bc0db7/dialog/view.go#L26)

Other than that looking good to me. Cheers!

RamiAwar commented 8 months ago

Hi @RamiAwar

I also am not a big fan of the switching of parameters as is - it is still lacking visibility. I want to see what other options I have (as in a kind of drop down with highlighting of some sort).

Perhaps easiest would be to add an optional clue when there are multiple values: image

(sample code: https://github.com/jaroslawhartman/pet/blob/94ff848e4cd364cca28021ae4f9cddea28bc0db7/dialog/view.go#L26)

Other than that looking good to me. Cheers!

Love this idea. I was overcomplicating my life by trying to do a select dropdown to cycle through the options but this is a great MVP. Thanks for suggesting it!

morriswinkler commented 8 months ago

@RamiAwar

Sorry for the late reply, good job, I agree it is a good simple start.

I don't think you can do select boxes with gocui, that was the reason I just used the up/down key solution.

But anyway thanks for finally adding that.

RamiAwar commented 8 months ago

Thanks @morriswinkler! Wouldn't have possible without your contributions 😃

We can and will do a select dropdown with gocui actually 😉 Will just need some more gocui knowledge on my part but I already built a scrappy mvp while I was exploring this ticket.

image dropdown-demo

Soon!