Add new splitQuery API accepting Ktor Url or string#2931
Conversation
fire-light42
left a comment
fire-light42
left a comment
There was a problem hiding this comment.
It works well from testing and the code looks good 👍
I just have a thought regarding the new API surface. This is not blocking. I just want to hear what you think before merging this.
| * // returns {"foo": "bar", "baz": "qux"} | ||
| */ | ||
| @Prerelease | ||
| fun splitQuery(url: Url): Map<String, String> { |
There was a problem hiding this comment.
I would prefer to keep the API-surface somewhat dependency-agnostic. There is no need to make the same mistake as splitQuery(url: java.net.URL) again, when we can just expose a string argument instead.
There was a problem hiding this comment.
What I was aiming for was to expose a way to accept a direct URL as well as String. By passing a URL it can sometimes be a bit more type safe as well, and also as we shift towards ktor URL more, sometimes a URL may be wanted directly, in which case, I just thought it was better and more straightforward to accept a URL with a separate convenience method accepting a string only as well. I am open to changes though if you think it should be done differently?
There was a problem hiding this comment.
I am kinda considering whether this should instead be an extension function on String in StringUtils though, in which case it would make a little more sense to simply drop the URL overload and just accept that extension method. I am honestly not 100% sure on the best option here...
There was a problem hiding this comment.
Hmm. Maybe better not in StringUtils (or is it, still haven't decided that) but perhaps it also being called something like splitUrlParameters or getUrlParameters would make more sense and be more clear than splitQuery?
There was a problem hiding this comment.
I am thinking maybe just name splitQuery, parseUrlParameters instead. It's a more clear naming scheme, and is more consistent with other Url-based naming.
2 participants
No description provided.