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

Rename to url command to url build-query #7702

Merged
merged 3 commits into from
Jan 15, 2023

Conversation

VincenzoCarlino
Copy link
Contributor

@VincenzoCarlino VincenzoCarlino commented Jan 7, 2023

Description

Refactor command: "to url" in: "to url query". Changed usage sentence.
Closes: #7495

User-Facing Changes

Now we get a query string from a record or table by using command: "to url query".

> help to url query
Convert record or table into query string applying percent-encoding.

Usage:
  > to url query

Flags:
  -h, --help - Display the help message for this command

Signatures:
  <record> | to url query -> <string>
  <table> | to url query -> <string>

Examples:
  Outputs a query string representing the contents of this record
  > { mode:normal userid:31415 } | to url query

  Outputs a query string representing the contents of this 1-row table
  > [[foo bar]; ["1" "2"]] | to url query

  Outputs a query string representing the contents of this record
  > {a:"AT&T", b: "AT T"} | to url query

Tests + Formatting

Added this test:

Example {
    description: "Outputs a query string representing the contents of this record",
    example: r#"{a:"AT&T", b: "AT T"} | to url query"#,
    result: Some(Value::test_string("a=AT%26T&b=AT+T")),
},

to ensure percent-encoding.

After Submitting

If PR is accepted I'll open another PR on documentation to notify changes on this.

@fdncred
Copy link
Collaborator

fdncred commented Jan 7, 2023

I'm not a fan of the command name to url query. I don't think we have any other commands with three words. I'm wondering if this should be part of url encode like url encode --query-str, since both command do percent encoding.

Maybe like this

'{ mode:normal userid:31415 }' | url encode --query-str

@VincenzoCarlino
Copy link
Contributor Author

Ok I got your point and it make sense, later I'll do it.

@VincenzoCarlino
Copy link
Contributor Author

@fdncred Just thinking, you said that commands with three words is not a good option and I get it; but I'm thinking if a command like: to query-str could do this job. I saw commands with name bigger than this, for example: path relative-to or str screaming-snake-case etc.
I'm pointing out this because I'm going down on url encode and I think that maybe adding arg --query-str could add a bit of confusion. In genearl url encode is used with a string (that should "represent" an url or part of it), instead if we call it with arg --query-str we should pass a string representing a Record or Table and this could lead users to make errors.
What do you think?

P.S. I'm new on the repo so I'm sorry if I'm missing something or if I made some conceptual error in my explanation :)

@fdncred
Copy link
Collaborator

fdncred commented Jan 7, 2023

I thought about to query-str too but I just thought it was a better fit for url encode since they both do percent encoding. Let's see what others think.

@VincenzoCarlino
Copy link
Contributor Author

Ok tnx for the reply, I'll wait for further indications.

@rgwood
Copy link
Contributor

rgwood commented Jan 7, 2023

I'm wondering if this should be part of url encode like url encode --query-str, since both command do percent encoding.

I feel very strongly that url encode is the wrong place for this functionality.

I would support to query-string or something new under url (url build-query?).

@VincenzoCarlino
Copy link
Contributor Author

I'm wondering if this should be part of url encode like url encode --query-str, since both command do percent encoding.

I feel very strongly that url encode is the wrong place for this functionality.

I would support to query-string or something new under url (url build-query?).

url build-query sounds good to me

@VincenzoCarlino
Copy link
Contributor Author

VincenzoCarlino commented Jan 10, 2023

Any update? We can go with url build-query?

@fdncred
Copy link
Collaborator

fdncred commented Jan 10, 2023

We can go with url build-query?

sure, that's fine.

@rgwood
Copy link
Contributor

rgwood commented Jan 10, 2023

Yeah, let's go with url build-query for now.

cc: @webbedspace who may have another suggestion

@webbedspace
Copy link
Contributor

webbedspace commented Jan 11, 2023

I don't like the word build because the only other thing that uses build is bytes build, which ignores input and has a rather different signature. (Personally I think bytes build is a very strange command that really should just be replaced with ++ or something, but w/e.)

@rgwood
Copy link
Contributor

rgwood commented Jan 11, 2023

@VincenzoCarlino since we don't have any better ideas right now, feel free to go with url build-query. That's an improvement on the current state, and we can rename it again later if needed.

@VincenzoCarlino
Copy link
Contributor Author

Hi, I've pushed the changes to create command url build-query and removed previous command to url query (and to url).
I'm not sure which is the best category for this command, currently is set to: Network, let me know if it's ok.
Do I need to change PR title and description?

Tnx.

@VincenzoCarlino
Copy link
Contributor Author

VincenzoCarlino commented Jan 13, 2023

Oooops, on my machine with cargo run test all tests were ok, later I'll check and try to fix that check that fails

@webbedspace
Copy link
Contributor

You have to run cargo test --workspace instead of just cargo test, for some bizarre reason.

@sholderbach sholderbach changed the title Refactor "to url" command in: "to url query" Rename to url command to url build-query Jan 13, 2023
@sholderbach sholderbach added the pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes label Jan 13, 2023
@VincenzoCarlino
Copy link
Contributor Author

VincenzoCarlino commented Jan 15, 2023

Ok now with: cargo test --workspace all tests are ok.
I just figured out another "issue": I was with my personal pc, where system language is italian, so decimal separator is comma: , and not .; this example test will fail (and also some others below), because if the system use the comma as decimal separator, the output from this: 5 | into string -d 3 is :5,000 instead we expect: 5.000.

Copy link
Contributor

@rgwood rgwood left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the contribution!

@rgwood rgwood merged commit 92c4097 into nushell:main Jan 15, 2023
@VincenzoCarlino VincenzoCarlino deleted the refactor/7495-to-url branch January 15, 2023 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

to url needs a new name and help info
5 participants