Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve combine to not return lazy sequences #304

Merged
merged 1 commit into from
Jan 7, 2019
Merged

Improve combine to not return lazy sequences #304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
version which blocked updating some dependencies
* Update clojure versions in the build matrix.
* Allow `defresource` to have a docstring (#305)
* Improve `liberator.util/combine` to not return lazy sequences (#304)
Copy link
Member

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.

Copy link
Contributor Author

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.


## Bugs fixed

Expand Down
6 changes: 3 additions & 3 deletions src/liberator/util.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,9 @@
(cond
(-> newval meta :replace) newval
(and (map? curr) (map? newval)) (merge-with combine curr newval)
(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))
Copy link
Member

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.

Copy link
Contributor Author

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)

Copy link
Member

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?

Copy link
Contributor Author

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.

(and (vector? curr) (coll? newval)) (into curr newval)
(and (set? curr) (coll? newval)) (into curr newval)
:otherwise newval))

(defn is-protocol-exception?
Expand Down
4 changes: 4 additions & 0 deletions test/test_util.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,13 @@
(facts "combine function"
(facts "simple combinations"
(fact "merges map" (combine {:a 1} {:b 2}) => {:a 1 :b 2})
(fact "returns a map" (combine {:a 1} {:b 2}) => map?)
(fact "concats list" (combine '(1 2) [3 4]) => '(1 2 3 4))
(fact "returns a list" (combine '(1 2) [3 4]) => list?)
(fact "concats vector" (combine [1 2] '(3 4)) => [1 2 3 4])
(fact "returns a vector" (combine [1 2] '(3 4)) => vector?)
(fact "concats set" (combine #{1 2} [3 4]) => #{1 2 3 4})
(fact "returns a set" (combine #{1 2} [3 4]) => set?)
(facts "replaces other types"
(fact (combine 123 456) => 456)
(fact (combine "abc" 123) => 123)
Expand Down