Improve combine to not return lazy sequences
#304
Conversation
jeremyheiler
force-pushed
the
improve-combine-results
branch
3 times, most recently
from
3332e93
to
8d28e41
Compare
|
I think this needs a tests, so we can be sure we achieve the indented result. |
|
There is a comprehensive test here: https://github.com/clojure-liberator/liberator/blob/master/test/test_util.clj#L5 But maybe we could be more careful by testing sequences? Which made me think, should this function |
|
My reasoning was that when we change the implementation (except for performance) then we must also need to change a test -- or the test would be missing :) |
| (and (list? curr) (coll? newval)) (concat curr newval) | ||
| (and (vector? curr) (coll? newval)) (concat curr newval) | ||
| (and (set? curr) (coll? newval)) (set (concat curr newval)) | ||
| (and (list? curr) (coll? newval)) (apply list (concat curr newval)) |
There was a problem hiding this comment.
Why can't we use (into curr newval) here, too? If yes, this would simplify non-map-collections to a single clause.
There was a problem hiding this comment.
My reasoning was to preserve backwards compatibility for insertion order.
user=> (into '(1 2 3) '(4 5 6))
(6 5 4 1 2 3)
user=> (apply list (concat '(1 2 3) '(4 5 6)))
(1 2 3 4 5 6)
There was a problem hiding this comment.
Great catch! Do we have a test for that?
There was a problem hiding this comment.
Yes, the current test fails otherwise.
| @@ -5,6 +5,7 @@ | |||
| * Remove old examples. These dependet on an ancient clojurescript | |||
| version which blocked updating some dependencies | |||
| * Update clojure versions in the build matrix. | |||
| * Improve `liberator.util/combine` to not return lazy sequences (#304) | |||
There was a problem hiding this comment.
I think combining to a lazy sequence in general is not bad. We just need to be sure that we don't make lazy results where not expected.
There was a problem hiding this comment.
Yeah, maybe for another PR! I'm also not sure the best way to check for a lazy seq, since seq? also returns true for lists.
The `combine` function attempts to add new values to a collection by merging them. However, for lists and a vectors, a lazy sequence is returned rather than a list or a vector. For lists, `(apply list (concat ...` is used instead of `(into ...` in order to preserve element order for backwards compatibility, as `into` would put the new elements at the head of the list. For vectors, `into` is used, as new elements are added to the tail. The implementation for sets is simplified.
jeremyheiler
force-pushed
the
improve-combine-results
branch
from
b14f3ea
to
576f64a
Compare
|
Thanks for the patch! |
|
@jeremyheiler - Thanks for patching this issue! This notification was a nice surprise to find after getting back from vacation this morning! 😄 |
…mbine-results Improve `combine` to not return lazy sequences
The
combinefunction attempts to add new values to a collection bymerging them. However, for lists and a vectors, a lazy sequence is
returned rather than a list or a vector.
For lists,
(apply list (concat ...is used instead of(into ...inorder to preserve element order for backwards compatibility, as
intowould put the new elements at the head of the list.
For vectors,
intois used, as new elements are added to the tail.The implementation for sets is simplified.
This resolves #293.