Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: allow to set several parameters #239

Closed
wants to merge 3 commits into from

Conversation

morriswinkler
Copy link

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
Copy link
Author

morriswinkler commented Jan 26, 2024

@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 #218 (comment) so I ping you too @RamiAwar

@RamiAwar
Copy link
Collaborator

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

@RamiAwar
Copy link
Collaborator

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 RamiAwar@9150f79, just want to see which is a better solution and merge it in.

@RamiAwar RamiAwar added the feature-request Feature Request label Jan 30, 2024
@morriswinkler
Copy link
Author

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Feature Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants