Datomic blueprints implementation #238

Closed
wants to merge 19 commits into
from

Conversation

Projects
None yet
3 participants
@datablend

No description provided.

@spmallette

This comment has been minimized.

Show comment
Hide comment
@spmallette

spmallette Apr 11, 2012

Member

Thanks for your contribution. I just gave a quick glance to review this pull request from a Blueprints API implementation consistency perspective. One thing I noticed here:

https://github.com/tinkerpop/blueprints/pull/238/files#L3R76

The Element setProperty method should throw an IllegalArgumentException if the value is not a valid type expected by the underlying graph. If I'm following the diff right, the IllegalArgumentException is thrown by way of a call from here:

https://github.com/tinkerpop/blueprints/pull/238/files#L7R58

Is that correct?

Member

spmallette commented Apr 11, 2012

Thanks for your contribution. I just gave a quick glance to review this pull request from a Blueprints API implementation consistency perspective. One thing I noticed here:

https://github.com/tinkerpop/blueprints/pull/238/files#L3R76

The Element setProperty method should throw an IllegalArgumentException if the value is not a valid type expected by the underlying graph. If I'm following the diff right, the IllegalArgumentException is thrown by way of a call from here:

https://github.com/tinkerpop/blueprints/pull/238/files#L7R58

Is that correct?

@datablend

This comment has been minimized.

Show comment
Hide comment
@datablend

datablend Apr 11, 2012

That is correct. Just tested it out to make sure:

Vertex davy = graph.addVertex(null);
davy.setProperty("name",new Test());

Exception in thread "main" java.lang.IllegalArgumentException: Object type com.tinkerpop.blueprints.pgm.impls.datomic.Test not supported
at com.tinkerpop.blueprints.pgm.impls.datomic.util.DatomicUtil.mapJavaTypeToDatomicType(DatomicUtil.java:54)
at com.tinkerpop.blueprints.pgm.impls.datomic.util.DatomicUtil.createKey(DatomicUtil.java:85)
at com.tinkerpop.blueprints.pgm.impls.datomic.util.DatomicUtil.existingAttributeDefinition(DatomicUtil.java:76)
at com.tinkerpop.blueprints.pgm.impls.datomic.util.DatomicUtil.createAttributeDefinition(DatomicUtil.java:59)
at com.tinkerpop.blueprints.pgm.impls.datomic.DatomicElement.setProperty(DatomicElement.java:84)
at com.tinkerpop.blueprints.pgm.impls.datomic.Test.main(Test.java:26)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
at java.lang.reflect.Method.invoke(Method.java:597)
at com.intellij.rt.execution.application.AppMain.main(AppMain.java:120)

That is correct. Just tested it out to make sure:

Vertex davy = graph.addVertex(null);
davy.setProperty("name",new Test());

Exception in thread "main" java.lang.IllegalArgumentException: Object type com.tinkerpop.blueprints.pgm.impls.datomic.Test not supported
at com.tinkerpop.blueprints.pgm.impls.datomic.util.DatomicUtil.mapJavaTypeToDatomicType(DatomicUtil.java:54)
at com.tinkerpop.blueprints.pgm.impls.datomic.util.DatomicUtil.createKey(DatomicUtil.java:85)
at com.tinkerpop.blueprints.pgm.impls.datomic.util.DatomicUtil.existingAttributeDefinition(DatomicUtil.java:76)
at com.tinkerpop.blueprints.pgm.impls.datomic.util.DatomicUtil.createAttributeDefinition(DatomicUtil.java:59)
at com.tinkerpop.blueprints.pgm.impls.datomic.DatomicElement.setProperty(DatomicElement.java:84)
at com.tinkerpop.blueprints.pgm.impls.datomic.Test.main(Test.java:26)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
at java.lang.reflect.Method.invoke(Method.java:597)
at com.intellij.rt.execution.application.AppMain.main(AppMain.java:120)

@datablend datablend closed this Apr 11, 2012

@datablend datablend reopened this Apr 11, 2012

@ohpauleez

This comment has been minimized.

Show comment
Hide comment
@ohpauleez

ohpauleez Jun 10, 2012

I would love to see this get merged in

I would love to see this get merged in

@spmallette

This comment has been minimized.

Show comment
Hide comment
@spmallette

spmallette Jul 13, 2012

Member

I think we should consider options for getting Datomic into mainline after we release 2.1 in the coming weeks. Please let me know if you think there is anything we need to think about or consider in terms of doing that.

Member

spmallette commented Jul 13, 2012

I think we should consider options for getting Datomic into mainline after we release 2.1 in the coming weeks. Please let me know if you think there is anything we need to think about or consider in terms of doing that.

@datablend

This comment has been minimized.

Show comment
Hide comment
@datablend

datablend Jul 13, 2012

Not a lot I guess. The only disadvantage I can think off is that people need to manually install the datomic dependency.

Not a lot I guess. The only disadvantage I can think off is that people need to manually install the datomic dependency.

@spmallette

This comment has been minimized.

Show comment
Hide comment
@spmallette

spmallette Oct 10, 2012

Member

after a number of discussions on tinkerpop-contributors, it appears we've basically decided to keep your blueprints implementation separate from the mainline of blueprints development. is it ok to close this pull request?

from there it would be cool to collaborate with you to build rexster configuration classes in your repos. i think i could do that pretty quickly for you if you can talk to me a little bit about what the configuration options are.

Member

spmallette commented Oct 10, 2012

after a number of discussions on tinkerpop-contributors, it appears we've basically decided to keep your blueprints implementation separate from the mainline of blueprints development. is it ok to close this pull request?

from there it would be cool to collaborate with you to build rexster configuration classes in your repos. i think i could do that pretty quickly for you if you can talk to me a little bit about what the configuration options are.

@datablend

This comment has been minimized.

Show comment
Hide comment
@datablend

datablend Oct 10, 2012

Ok. Will close this pull request and create a new repository specifically for FluxGraph.

Ok. Will close this pull request and create a new repository specifically for FluxGraph.

@datablend datablend closed this Oct 10, 2012

@spmallette

This comment has been minimized.

Show comment
Hide comment
@spmallette

spmallette Oct 10, 2012

Member

will FluxGraph cover both the mongodb and dataomic implementations?

Member

spmallette commented Oct 10, 2012

will FluxGraph cover both the mongodb and dataomic implementations?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment