[Request for Comments] Changing behavior of `query-map` / `query-string->map` on empty query string

[plexus]

PR #46 changed the behavior of query-map / query-string->map. Before, when called on an empty query string, we would always return nil.

(query-map "https://example.com) ;;=> nil

But query-map accepts an :into parameter, and it is reasonable to expect that the return value is of the same type.

;; old behavior
(query-map "https://example.com" {:into (sorted-map)}) ;; => nil
;; PR #46
(query-map "https://example.com" {:into (sorted-map)}) ;; => #sorted {}

This also changes the behavior when no :into is provided.

;; old behavior
(query-map "https://example.com") ;; => nil
;; PR #46
(query-map "https://example.com") ;; => {}

Which could be problematic, e.g. it's not at all unlikely that code like this exists "in the wild"

(if-let [m (query-map uri)]
  ,,,)

It would not be acceptable for us to break that.

I've made a follow-up PR (#49) that partially reverts the behavior. Return nil when called withot :into, return the into collection if it is provided.

;; old behavior
(query-map "https://example.com" {:into (sorted-map)}) ;; => nil
;; PR #46
(query-map "https://example.com" {:into (sorted-map)}) ;; => #sorted {}

This also changes the behavior when no :into is provided.

(query-map "https://example.com") ;; => nil
(query-map "https://example.com" {:into (sorted-map)}) ;; => #sorted {}

However that is still a breaking change that could potentially impact users. I think the impact of this would be small enough that I'm willing to risk it, but I'm hesitant. Hence this issue to collect some more points of view.

cc @velios

[paulschun]

As I understand it, the :into key was introduced before PR#46. If that's the case, I cast a vote for PR#49 as the most suitable for the goal of minimizing breaking changes while maximizing expected behavior.

It's fine if query-map returns nil without an :into option (and especially with no options map at all) - there's enough nil punning in the community that it's very much within the realm of expectation. Even if I'd also personally prefer an empty map like @velios I'd never raise a stink about it.

However, I'd expect to see whatever I passed into :into again if no query parameters were found. Even moreso if it wasn't an empty collection (which it may do already - sorry, no REPL on me!).

[barrosfelipe]

I agree with @paulschun that #49 seems reasonable, but:

However, I'd expect to see whatever I passed into :into again if no query parameters were found. Even moreso if it wasn't an empty collection (which it may do already - sorry, no REPL on me!).

I have just tried it out and the return value is nil even if a non-empty collection is passed to query-map's :into. This is surprising to me as well and I feel that it could be addressed in the same set of changes. What do you think @plexus ?

[daTobber]

Just my two cents, PR#49 works for me and would get my vote. A bit selfish since I currently use query-map only without :into.

[daveliepmann]

A bugfix with minimal breakage (like #49) seems like a good idea. Any remaining concerns (i.e. arity-1 query-map returning {} instead of nil, which would be nice) can be addressed in a major version bump or other breaking-change-notifying method.

[velios]

At the moment, I don’t understand what use is intended for into arg in query-string->map can be used, so I would suggest excluding this arg from the function. I described in detail why I think so in conversation of another PR. Perhaps I will change my opinion when if I receive a comment(from @plexus i guess) why this functionality was introduced. But if such a proposal were accepted, it would completely eliminate the need for both #46 and #49.

But these are pretty major changes that probably won't happen. Of the less serious changes #49, this is a compromise; unfortunately, it does not make it possible to reliably use the library to preserve the order of request arguments.

[velios]

Now, when #51 is merged, I can no longer find use-cases where I can use into param. For this reason, I suggest rolling back my patch #46 , due to what it adds non-obvious behaviour for not a lot of benefit.

[plexus]

Thanks for the input everyone, we're going with the intermediate option of returning an :into param if it's present, and nil (on blank input) if not.