knqyf263 / pet

Simple command-line snippet manager
MIT License
4.44k stars 223 forks source link

Enhance using same params with different default values #229

Closed rampollaluis closed 7 months ago

rampollaluis commented 11 months ago

Issue #, if available:

159

Description of changes: Was facing the same annoyance as issue.

dialog.SearchForParams now checks if a parameter with the same name has already been added to the resulting collection before inserting. Since it keys on the parameter name and not the parameter name + default value combination, defining 2 different defaults for the same parameter name eg. echo “<param1=Hello>, <param1=World>” would only return 1 param of default value “Hello” (Before it would return a param1 of default value “Hello” and another param1 with default value “World”).

This of course has the effect that if a param name is repeated in the same command, you only see it once in the exec dialog.

insertParams now needs to replace all instances of the same param name regardless of whether it has different or no default value. To do this, I made a function to normalize the parameters with defaults in the command to just their names.

I have made the assumption that the exec form showing \<param-name> in the top of each input instead of just param-name was deliberate. If it was not and you are ok with it just saying param-name, the implementation would be a bit simpler.

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

RamiAwar commented 7 months ago

Okay I think I see the point of this. But I feel like having different default values for the same parameter name is kind of weird. If it's the parameter has the same name, then I'd expect it to want the same default value.

Otherwise just name the parameter differently, no?

Also, another PR was merged recently that fixes some issues with parameter default value replacement. Now only the latest default value is considered (if it is non-empty).

RamiAwar commented 7 months ago

Hey @rampollaluis , gonna close this for now but feel free to open a discussion if you want to discuss that last idea further! The latest version of pet has some of the changes you did, just implemented a bit differently.