Skip to content

provide a common implementation for query string conversions in url join and url build-query#14173

Merged
fdncred merged 5 commits intonushell:mainfrom
Bahex:url
Oct 29, 2024
Merged

provide a common implementation for query string conversions in url join and url build-query#14173
fdncred merged 5 commits intonushell:mainfrom
Bahex:url

Conversation

@Bahex
Copy link
Copy Markdown
Member

@Bahex Bahex commented Oct 27, 2024

Addresses one of the points in #14162

Description

Factors out part of the url::build_query::to_url function into a separate function url::query::record_to_qs(), which is then used in both url::build_query and url::join.

User-Facing Changes

Like with url build-query (after #14073), url join will allow list values in params and behavior of two commands will be same.

> {a: ["one", "two"], b: "three"} | url build-query
"a=one&a=two&b=three"

> {scheme: "http", host: "host", params: {a: ["one", "two"], b: "three"}} | url join 
"http://host?a=one&a=two&b=three"

Tests + Formatting

Added an example to url join for the new behavior.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Oct 27, 2024

Thanks. I like the consistency.

What does qs stand for in record_to_qs? Whatever it is, it may be better to spell it out. Also, it would be nice if the examples had different descriptions so that a user can see what the difference is that we're trying to convey instead of them all saying, "Outputs a url representing the contents of this record". I realize you didn't create all these, but it would be nice to clean it up a little bit while you're in here.

@Bahex
Copy link
Copy Markdown
Member Author

Bahex commented Oct 27, 2024

qs was already used as a local variable name in some places so I just went with it. But spelling it out in a free-standing function makes more sense.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Oct 27, 2024

ah, query string. makes sense now that i know what it is. thanks for cleaning up.

@fdncred fdncred merged commit 719d9aa into nushell:main Oct 29, 2024
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Oct 29, 2024

Thanks

@github-actions github-actions bot added this to the v0.100.0 milestone Oct 29, 2024
@fdncred fdncred added the deprecated:pr-commands (deprecated: too vague) This PR changes our commands in some way label Nov 4, 2024
@Bahex Bahex deleted the url branch March 22, 2026 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

deprecated:pr-commands (deprecated: too vague) This PR changes our commands in some way

Projects

None yet

Milestone

v0.100.0

Development

Successfully merging this pull request may close these issues.

2 participants

@Bahex @fdncred