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

Query fromString to unsafeFromString #4581

Merged
merged 1 commit into from Mar 9, 2021
Merged

Query fromString to unsafeFromString #4581

merged 1 commit into from Mar 9, 2021

Conversation

hamnis
Copy link
Contributor

@hamnis hamnis commented Mar 3, 2021

No description provided.

Copy link
Member

@m-sp m-sp left a comment

Choose a reason for hiding this comment

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

hmm in contrast to path we do not have an interpolator for the query string so I'm a bit more hesitant

@@ -197,14 +197,18 @@ object Query {
*
* If parsing fails, the empty [[Query]] is returned
*/
def fromString(query: String): Query =
def unsafeFromString(query: String): Query =
Copy link
Member

Choose a reason for hiding this comment

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

If we're calling this unsafe, does it still make sense to quietly return an empty? And might fromString now return an Either?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about doing that in two steps. first deprecating, then change the return type.
We could however do both in one step.

Copy link
Member

Choose a reason for hiding this comment

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

I think we'll probably want to reclaim the real name fromString for 0.22 and 1.0, so we should probably just go ahead and do it in the next milestone. But the change could be part of a subsequent PR.

@rossabaker
Copy link
Member

@m-sp's point on an interpolator is interesting. I found during the ip4s work that hiding an interpolator when it clashes is really hard! So we should be conservative about the ones we create. Or at least be pretty hopeful nobody else will use the same name. uriQuery is probably too verbose. Is query unique enough? Or should we live without?

Copy link
Member

@m-sp m-sp left a comment

Choose a reason for hiding this comment

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

Is query unique enough

most likely not, I'n okay merging this but it is getting a bit hard to just construct a correct uri from scratch. especially without using any unsafe methods. as can be seen in our tests

@hamnis
Copy link
Contributor Author

hamnis commented Mar 5, 2021

If we had a Uri interpolator which accepted parameters, then we would be in a much better place with regards to uri construction.

Using the unsafe constructors are not bad per se, but we cant guarantee that every string is a "blessed" string without going through some parsing step. If you are sure that the string does not contain badness, then its fine.

@rossabaker
Copy link
Member

A good URI interpolator is complicated, but is one of the highest value things we could add right now.

@rossabaker
Copy link
Member

👍, but has some merge conflicts

@hamnis hamnis merged commit 65d432b into http4s:series/0.22 Mar 9, 2021
@hamnis hamnis deleted the query-fromString-to-unsafeFromString branch March 9, 2021 13:46
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.

None yet

3 participants