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

Would a url decode command be welcome? #10563

Closed
lpchaim opened this issue Oct 1, 2023 · 7 comments · Fixed by #10611
Closed

Would a url decode command be welcome? #10563

lpchaim opened this issue Oct 1, 2023 · 7 comments · Fixed by #10611
Labels
new-command question the issue author asks something
Milestone

Comments

@lpchaim
Copy link
Contributor

lpchaim commented Oct 1, 2023

Question

Hey all, I've been wondering if a PR implementing a url decode comand would be welcome, essentially doing the opposite of the existing url encode implemented here. Unless I'm missing something, said feature doesn't exist yet so my intention is to go ahead and implement it if this sounds reasonable, as per the contribution guide.

Additional context and details

No response

@lpchaim lpchaim added the question the issue author asks something label Oct 1, 2023
@fdncred
Copy link
Collaborator

fdncred commented Oct 1, 2023

I wouldn't have a problem with it. Not sure what other maintainers think though. Seems complimentary to me.

@sophiajt
Copy link
Contributor

sophiajt commented Oct 1, 2023

This sounds similar to from url. It's this similar to that? If so, maybe the current command needs a better name

@amtoine
Copy link
Member

amtoine commented Oct 1, 2023

then should url decode be called to url? 🤔
and what about the other url ... commands? 😕

maybe having all these in a special url command can still make sense 😋

@fdncred
Copy link
Collaborator

fdncred commented Oct 1, 2023

if that's what from url does, then i'd support renaming it to url decode. Seems much more discoverable having it in the family of url commands.

@lpchaim
Copy link
Contributor Author

lpchaim commented Oct 1, 2023

This sounds similar to from url. It's this similar to that? If so, maybe the current command needs a better name

I might be missing something but I can't find said from url command and going by #6854, it'd seem it was one of the proposed names for the final url parse command. Assuming that's what you're referring to, it's not quite the same. url parse creates a record from a URL string, while the proposed url decode would simply transform an input string into a new, decoded string. In fact, I just checked and from url seems to break when supplied with URL encoded parameters, so it doesn't seem at all prepared to deal with that. Not that I'm trying to argue it should, necessarily, but maybe it's also something to consider.

then should url decode be called to url? 🤔 and what about the other url ... commands? 😕

maybe having all these in a special url command can still make sense 😋

With the above context, I personally still believe url decode would be more appropriate and also more symmetrical with the existing commands. I might be biased since I'm a web developer by trade but having the url commands on their own namespace seemed like a natural fit when I learned about them, since URLs have quite unique, particular behavior.

@amtoine
Copy link
Member

amtoine commented Oct 1, 2023

With the above context, I personally still believe url decode would be more appropriate and also more symmetrical with the existing commands. I might be biased since I'm a web developer by trade but having the url commands on their own namespace seemed like a natural fit when I learned about them, since URLs have quite unique, particular behavior.

sounds fair to me 👍

also, i feel like from ... and to ... are mostly there for file formats, so one (me as well) could argue that URLs are not file formats and thus should not be added under these 😏

@lpchaim
Copy link
Contributor Author

lpchaim commented Oct 1, 2023

Had a free morning so I've come up with and performed initial tests on an implementation, I reckon I'm going to polish it some more then go ahead and make a PR within the next few days if no further issues are raised. Hopefully that's the right way to go about this, it's my first time contributing so I'm trying to be extra mindful and not too annoying 😅

@lpchaim lpchaim mentioned this issue Oct 4, 2023
4 tasks
amtoine pushed a commit that referenced this issue Oct 5, 2023
Implemented URL decoding as a url subcommand, created corresponding unit
tests. The logic, examples and descriptions were based on the existing
`url encode` command.

Resolves #10563

# Description
Added a new `url decode` command to compliment the existing `url
encode`, as proposed by myself in #10563.
It takes a string, list of strings or cell path and produces the
corresponding decoded strings.

![image](https://github.com/nushell/nushell/assets/4030336/815a34e9-7ceb-4d09-9d74-e700ba513b17)

# User-Facing Changes
New url subcommand `url decode`, as described above.

# Tests + Formatting
I've added unit tests for the new subcommand and ensured all actions
outlined below showed no issues.
- [x] `cargo fmt --all -- --check`
- [x] `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used`
- [x] `cargo test --workspace`
- [x] `cargo run -- -c "use std testing; testing run-tests --path
crates/nu-std"`
@hustcer hustcer added this to the v0.86.0 milestone Oct 5, 2023
hardfau1t pushed a commit to hardfau1t/nushell that referenced this issue Dec 14, 2023
Implemented URL decoding as a url subcommand, created corresponding unit
tests. The logic, examples and descriptions were based on the existing
`url encode` command.

Resolves nushell#10563

# Description
Added a new `url decode` command to compliment the existing `url
encode`, as proposed by myself in nushell#10563.
It takes a string, list of strings or cell path and produces the
corresponding decoded strings.

![image](https://github.com/nushell/nushell/assets/4030336/815a34e9-7ceb-4d09-9d74-e700ba513b17)

# User-Facing Changes
New url subcommand `url decode`, as described above.

# Tests + Formatting
I've added unit tests for the new subcommand and ensured all actions
outlined below showed no issues.
- [x] `cargo fmt --all -- --check`
- [x] `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used`
- [x] `cargo test --workspace`
- [x] `cargo run -- -c "use std testing; testing run-tests --path
crates/nu-std"`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-command question the issue author asks something
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants