curl / trurl

trurl is a command line tool for URL parsing and manipulation.
https://curl.se/trurl/
Other
3.1k stars 99 forks source link

`--set "query:$NAME=$VALUE"` is not supported #303

Closed yahesh closed 2 months ago

yahesh commented 3 months ago

While --get "{query:$NAME}" is valid syntax, --set "query:$NAME=$VALUE" is not. This is somewhat unintuitive. Instead, you have to use --force-replace "$NAME=$VALUE" which does the same thing as --set for all other URL components.

bagder commented 3 months ago

You can do it like this:


$ trurl example.com --set query:=name=moo
http://example.com/?name=moo
yahesh commented 3 months ago

You can do it like this:

$ trurl example.com --set query:=name=moo
http://example.com/?name=moo

But this will replace the whole query string instead of just setting the specific query parameter:

% trurl "https://localhost?name1=value1&name2=value2" --set query:=name1=newvalue
https://localhost/?name1=newvalue

From a consistency perspective I'd expect something like:

% trurl "https://localhost?name1=value1&name2=value2" --get "{query:name1}"
value1

% trurl "https://localhost?name1=value1&name2=value2" --set "query:name1=newvalue"
https://localhost/?name1=newvalue&name2=value2
bagder commented 3 months ago

Ah, thanks, now I understand what you meant!

bagder commented 3 months ago

This is what --replace does, is it not?


$ trurl "https://localhost?name1=value1&name2=value2" --replace name1=moo
https://localhost/?name1=moo&name2=value2
yahesh commented 3 months ago

This is what --replace does, is it not?

Yes, or --force-replace. However, as a user, I'd expect --set "query:$NAME=$VALUE" to work as well, which would syntactically be more in line with how the content of all other components is replaced (especially as there is no query-specific option to get named query parameters either, instead --get is used for this like for all other components).

This issue is more about syntactical consistency than available functionality.

bagder commented 3 months ago

I'm not sure such strict consistency is worth fighting for.

--set "query:$NAME=$VALUE" would then need to do either --replace or --force-replace so we would have to invent another little tweak for the alternative.

--replace only replaces a query name that is already used in the URL, while --force-replace will replace it or if not existing already - add it.

yahesh commented 3 months ago

--set "query:$NAME=$VALUE" would then need to do either --replace or --force-replace so we would have to invent another little tweak for the alternative.

I don't agree here. I'd personally expect --set to always set a given value (equal to --force-replace) even if it is not yet available. This would be equivalent to the handling of the port number for schemes that are unknown (which also happens to work like --force-replace).

% trurl "unknown://localhost" --get "{query:name}"

% trurl "unknown://localhost" --set "query:name=value"
unknown://localhost/?name=value

would then behave identically to

% trurl "unknown://localhost" --get "{port}"

% trurl "unknown://localhost" --set "port=123"
unknown://localhost:123/

This would allow --force-replace to be deprecated as it is currently only used for this one case and e.g. doesn't properly support multiple query parameters and instead produces unexpected results like this:

trurl "unknown://localhost?name1=value1&name2=value2" --force-replace "name1=newvalue1&name2=newvalue2"
unknown://localhost/?name1=newvalue1&name2=newvalue2&name2=value2
jacobmealey commented 3 months ago

I think --force-replace is a bit ugly, and probably needs better docs around how to use it. it should handle something like that last example in some way. I wonder if instead it should be handled by --replace and do something like --replace thing!=newval where the ! is like vi force operator -- though this is confusing because != looks a lot more like not equals than force equals, so maybe a different symbol should be use.

Having --set replace the whole query makes sense though, because you are setting the query not a parameter. --set can work as you've descrived for ports and schemes because there can only be one. If I had a bunch of urls with a bunch of arbitrary queries (for example something.com/foo=fum&query=string&anotherquery) and i wanted it to only be foo=bar how would you recommend this happens? With your proposal I only see it working by using --trim to remove the other queries (and if they are arbitrary queries that becomes a pretty challenging problem).

I suppose we could have --set take a special character for replacing queries but we are going to quickly run out of special characters if we try to have every operation covered in --set

yahesh commented 3 months ago

Having --set replace the whole query makes sense though, because you are setting the query not a parameter. --set can work as you've descrived for ports and schemes because there can only be one. If I had a bunch of urls with a bunch of arbitrary queries (for example something.com/foo=fum&query=string&anotherquery) and i wanted it to only be foo=bar how would you recommend this happens? With your proposal I only see it working by using --trim to remove the other queries (and if they are arbitrary queries that becomes a pretty challenging problem).

I don't talk about taking the possibility away to replace the whole query via --set "query:=foo=bar" which is the counterpart to --get "{query}". I talk about --set "query:foo=bar" (or --set "query:foo:=bar") being the counterpart to --get "{query:foo}" (or --get "{:query:foo}").

jacobmealey commented 3 months ago

Right, but if we have --set "query:foo=bar be the counterpart to --get "{query:foo}" where it only does operations on the specified query parameter how would we replace the whole query?

yahesh commented 3 months ago

Right, but if we have --set "query:foo=bar be the counterpart to --get "{query:foo}" where it only does operations on the specified query parameter how would we replace the whole query?

Via --set "query:=foo=bar" (or --set "query=foo=bar") just like now?

These are different syntaxes.

[component]  = [data] // sets the whole [component] and URL-encodes [data]
[component] := [data] // sets the whole [component] and does not URL-encode [data]

[component]:[key]  = [data] // sets a specific [key] of [component] and URL-encodes [data]
[component]:[key] := [data] // sets a specific [key] of [component] and does not URL-encode [data]

The same already holds true for the get option:

{[component]}  // gets the whole [component] and URL-decodes [data]
{:[component]} // gets the whole [component] and does not URL-decode [data]

{[component]:[key]}  // gets a specific [key] of [component] and URL-decodes [data]
{:[component]:[key]} // gets a specific [key] of [component] and does not URL-decode [data]
jacobmealey commented 3 months ago

Oh sorry, I misread --set "query:foo=bar" and kept reading it as --set "query:=foo=bar". I understand what you are saying now.

bagder commented 3 months ago

I would not be against if someone adds support for setting a specific query pair with --set as described here, but I am not going to push for it myself right now. Primarily because the functionality already exists even if the usability of it perhaps could be improved.