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

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

Open
yahesh opened this issue May 14, 2024 · 13 comments
Open

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

yahesh opened this issue May 14, 2024 · 13 comments

Comments

@yahesh
Copy link

yahesh commented May 14, 2024

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

bagder commented May 14, 2024

You can do it like this:

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

@yahesh
Copy link
Author

yahesh commented May 14, 2024

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

bagder commented May 14, 2024

Ah, thanks, now I understand what you meant!

@bagder
Copy link
Member

bagder commented May 14, 2024

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

yahesh commented May 14, 2024

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

bagder commented May 14, 2024

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

yahesh commented May 14, 2024

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

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

yahesh commented May 14, 2024

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

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

yahesh commented May 14, 2024

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

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

bagder commented May 17, 2024

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.

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

No branches or pull requests

3 participants