search

knqyf263 / pet

Simple command-line snippet manager
MIT License
4.41k stars 222 forks source link

Feat: allow to set several parameters #239

Closed morriswinkler closed 6 months ago

morriswinkler commented 7 months ago

Small feature to allow setting a list of default parameters separated by | in snippet command fields.

Includes also a bug fix that broke building on non windows systems. syscall.SysProcAttr on non windows systems doesn't contain the field CmdLine.

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

morriswinkler commented 7 months ago

@knqyf263 for the time being I won't fix the conflict, awaiting some feedback from you.

Don't know if you like that feature, I tried to make it backward compatible, so existing configs will behave as before. I just had many snippets where a set of defaults made so much sense.

I am a bit unsure about the delimiter since the | character is used so much in shell scripts, but I didn't come up with a character that is absolutely unlikely to be used.

You use also <([\S].+?[\S])> for params, which might break for example cat <<EOF some test EOF 2&> /dev/null, as described in #224.

Maybe it would make sense to be able to customise these characters a bit like sed does when creating an expression.

Just saw https://github.com/knqyf263/pet/issues/218#issuecomment-1799086031 so I ping you too @RamiAwar

RamiAwar commented 7 months ago

Hey Morris! This seems pretty cool, will take a look at it later today, thanks!

RamiAwar commented 7 months ago

Hey @morriswinkler, just checked this one out.

Could you give me an example of a snippet with multiple default values? I'm struggling to imagine a clear use case for this, and the pipe is a little problematic as you say as it could be part of the value itself.

As for the windows build bug fix, I'll try to verify that when I have access to my PC again next week. I'll let you know if we should put it in a separate PR after I check.

I'm also going to look into #224 soon as I've fixed something similar in my personal fork https://github.com/RamiAwar/superpet/commit/9150f799bb763f1e4a04fbe65f64172b116f7e24, just want to see which is a better solution and merge it in.

morriswinkler commented 6 months ago

Could you give me an example of a snippet with multiple default values? I'm struggling to imagine a clear use case for this, and the pipe is a little problematic as you say as it could be part of the value itself.

So I use the tool for interacting with deployed services, where i have the same command just replacing a service name or the region it is in. Before that change I had to change the names every time manually, now i just use up/down on my keyboard. Way faster.

As for the windows build bug fix, I'll try to verify that when I have access to my PC again next week. I'll let you know if we should put it in a separate PR after I check.

You won't have that issue if you build on windows, I wonder how the github action went with that.

About characters, I see | just as problematic as < and > which are already used.

I would propose to make those delimiters configurable in each snippet and otherwise keep it default, I would also change | to something else, just havn't really spend a lot of thought on it.

RamiAwar commented 6 months ago

Hey @morriswinkler, tried rebasing this but felt it was easier to just create a new PR, tagging you there!

I used your code to create a skeleton for supporting multiple default values.

Still needs some organizing, hacked this together very quickly just as a proof of concept.