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

Unroll :db.cardinality/many assertions #16

Merged
merged 1 commit into from
Aug 26, 2014
Merged

Unroll :db.cardinality/many assertions #16

merged 1 commit into from
Aug 26, 2014

Conversation

lynaghk
Copy link
Contributor

@lynaghk lynaghk commented Aug 26, 2014

With the latest (0.3.0) DataScript, :db.cardinality/many attributes are returned differently than they are in Datomic.

In DataScript:

(let [schema {:children {:db/cardinality :db.cardinality/many}}
      conn   (d/create-conn schema)]

      (d/transact! conn [{:db/id -1 :children #{"A" "B"}}])

      (->> (d/q '[:find ?e :where [?e :children _]]
                @conn)
           ffirst
           (d/entity @conn)
           :children)) ;;=> #{#{"B" "A"}}

whereas in Datomic the same code returns #{"B" "A"}.
Changing the transaction data to:

(d/transact! conn [{:db/id -1 :children "A"}
                   {:db/id -1 :children "B"}])

returns #{"A" "B"}.

If this is the desired behavior, would you be open to a pull request that makes

(d/transact! conn [{:db/id -1 :children #{"A" "B"}}])

equivalent to

(d/transact! conn [{:db/id -1 :children "A"}
                   {:db/id -1 :children "B"}])

@lynaghk
Copy link
Contributor Author

lynaghk commented Aug 26, 2014

I dug into this a bit more, and the problem is actually just that I used a set instead of a vector.
More specifically, the datascript.core/explode function checks with sequential? (

datascript/src/datascript/core.cljs

Line 154 in 5ad4116

v (if (and (sequential? vs)
).

I'll look into fixing this so that cardinality many attributes can accept sets, vectors, and lists.

@lynaghk
Attributes with :db.cardinality/many should accept all colleciton
c3076e1 
values.

Before this commit, they only accepted sequential values.
Now they accept everything that implements IPersistentCollection.
@lynaghk
Copy link
Contributor Author

lynaghk commented Aug 26, 2014

Converted this issue to a pull request.
This commit fixes the issue and updates the existing test to check for proper behavior with vectors, sets, and lists.

Please let me know if you'd like to see any changes in this commit.

@tonsky
Copy link
Owner

tonsky commented Aug 26, 2014

Yes, sure. One addition: I guess it should test for (and (coll? vs) (not (associative? vs))) so we filter out maps

tonsky added a commit that referenced this pull request Aug 26, 2014
@tonsky
Merge pull request #16 from lynaghk/bugfix-cardinality-many-colls
58c7615 
Accept sets in entity maps for :db.cardinality/many attributes
@tonsky tonsky merged commit 58c7615 into tonsky:master Aug 26, 2014
tonsky pushed a commit that referenced this pull request Aug 26, 2014
@lynaghk @tonsky
Attributes with :db.cardinality/many should accept all collection v…
c48c464 
…alues (#16)

Before this commit, they only accepted sequential values.
Now they accept everything that implements IPersistentCollection.
tonsky added a commit that referenced this pull request Aug 26, 2014
@tonsky
Do not treat maps as coll values for :db.cardinality/many attributes (c…
588d986 
…loses #16)
tonsky pushed a commit that referenced this pull request Aug 26, 2014
@lynaghk @tonsky
Attributes with :db.cardinality/many should accept all collection v…
c6c59ad 
…alues (#16)

Before this commit, they only accepted sequential values.
Now they accept everything that implements IPersistentCollection.
tonsky added a commit that referenced this pull request Aug 26, 2014
@tonsky
Do not treat maps as coll values for :db.cardinality/many attributes (c…
5a2724a 
…loses #16)
tonsky added a commit that referenced this pull request Aug 26, 2014
@tonsky
Do not treat maps as coll values for :db.cardinality/many attributes (c…
1d842bd 
…loses #16)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lynaghk @tonsky