lambdaisland / uri

A pure Clojure/ClojureScript URI library
Mozilla Public License 2.0
243 stars 21 forks source link

Change query-string->seq to query-string->vec #52

Closed velios closed 9 months ago

velios commented 9 months ago

I started testing #51 functionality and discovered some non-desired behavior. I've added an additional test case that shows the problem and also show example of usage query-string->seq fns.

We once discussed in other PR whether I had any reasons why a vector should be returned from query-string->seq and now I think answer is yes. I also renamed it to query-string->vec.

Non-desired behavior can reproduce with next steps. Change mapv to map in the query-string->vec function. After that new test case will return “b=2&a=1&c=3&d=4” instead of “c=3&d=4&a=1&b=2” with mapv, which is completely fail idea of usage new functions.

Reason of behaviour the same like in conj and cons fns.

  1. To avoid non-desired behaviour, I also again suggest returning a vector instead of a nil. (into nil ...) leads to unobvious results. I make it return nil to look the same as existing query-string->map fn. But now i think it's mislead user. If we have to return maybe change fn name to query-string->vec-or-nil. But personally i prefer query-string->vec and return an empty vector if the user passed a string without parameters
  2. I don't like what pair functions are called query-string->vec and seq->query-string but it reflects the meaning and I couldn’t figure out how to express it better
plexus commented 9 months ago

The current behavior is intentional and idiomatic. The result is a seq and so it behaves as one. Use concat or coerce the result to vec if necessary.