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

Add url decode command #10611

Merged
merged 1 commit into from Oct 5, 2023
Merged

Add url decode command #10611

merged 1 commit into from Oct 5, 2023

Conversation

lpchaim
Copy link
Contributor

@lpchaim lpchaim commented Oct 4, 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

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.

  • cargo fmt --all -- --check
  • cargo clippy --workspace -- -D warnings -D clippy::unwrap_used
  • cargo test --workspace
  • cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"

Implemented URL decoding as a url subcommand, created corresponding unit tests.
The logic, examples and descriptions were based on the complimentary url encode command.
Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this! Looks like the proper inverse for url encode doing the percent encoding.

Separately we should probably figure out what a good name for from url should be as this doesn't really belong among the file format parsers.

@lpchaim
Copy link
Contributor Author

lpchaim commented Oct 5, 2023

Thanks for tackling this! Looks like the proper inverse for url encode doing the percent encoding.

Absolutely, happy to contribute! That's exactly what I had in mind, yes.

Separately we should probably figure out what a good name for from url should be as this doesn't really belong among the file format parsers.

I realize now that my confusion at not being able to find from url among my commands when it was brought up in #10563 was due to it being an extra command, I'd somehow missed they were a thing so far.
Anyway, agreed. Taking a proper look at it, both the logic and code overlap with how url parse parses URL parameters so I feel making it a dedicated url subcommand named url parse-params or similar could work, assuming that's feasible for an extra. I've based the name on usual web API staples such as URLSearchParams so that it hopefully feels familiar to fellow web developers.
I'll give it some more thought and update if anything else comes to mind.

@amtoine
Copy link
Member

amtoine commented Oct 5, 2023

the code and the examples look good, thanks @lpchaim for making Nushell better 🙏

i propose we leave the renaming of from url to another PR, also because it's an extra command 😉

@amtoine amtoine merged commit a03c1c2 into nushell:main Oct 5, 2023
19 checks passed
hardfau1t pushed a commit to hardfau1t/nushell that referenced this pull request 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Would a url decode command be welcome?
3 participants