-
Notifications
You must be signed in to change notification settings - Fork 130
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
Improve combine
to not return lazy sequences
#304
Improve combine
to not return lazy sequences
#304
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! Do we have a test for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
combine
function 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
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.
This resolves #293.