search

Syncleus / Ferma

An ORM / OGM for the TinkerPop graph stack.
http://syncleus.com/Ferma
Apache License 2.0
137 stars 26 forks source link

Add @Incidence add method to support add* methods in Frames #1

Closed mohataher closed 9 years ago

mohataher commented 9 years ago

Hello,

As discussed in this thread, https://groups.google.com/a/syncleus.com/forum/#!topic/ferma-list/lrg-bzgyrl4 , I'm sending a pull request to add the functionality for @Incidence add* method.

This is really helpful to make it easier to merge from Frames.

One concern, this update forces full canonical name of Edge class. Though, it would be helpful to do it just by simple name but honestly, I couldn't find an easy way to get all annotated classes through the FramedGraph. If there is an easy way to get list of annotated edges, let me know, I will update the code and send a new pull request.

Thanks, Mohamed Taher Alrefaie

freemo commented 9 years ago

The ReflectionCache class can get you all annotated classes by subtype, so this is where that sort of information is typically stored. However I'm not sure what you meant when you said you dont want an update to force a full canonical name.

freemo commented 9 years ago

I did a bit looking into this pull request. Unfortunately it does not seem to support ClassInitializer arguments, which most of our add methods support. That will need to be added before i can accept the pull request.

freemo commented 9 years ago

The more I look into this the more issues I see. Aside from the lack of support of the various method types, like those supporting ClassInitializer there is also an issue with Direction = BOTH .. In which case two new edges are created but in this implementation only a single one is returned. I'm going to rewrite this commit to address some of these concerns, in which case I'll throw an exception in the case of BOTH. Any better ideas?

freemo commented 9 years ago

I merged this with a few edits to bring it up to spec with the rest of the library. Thanks for your contribution.

mohataher commented 9 years ago

That's a good question. I checked Frames and they throw any exception as well. Have a look here. https://github.com/tinkerpop/frames/blob/master/src/main/java/com/tinkerpop/frames/annotations/IncidenceAnnotationHandler.java#L42 Honestly, I wouldn't throw any exceptions but document this to ensure the developer knows what he's doing. But if we're working towards rebuilding Frames, so following their strategy would be better.

freemo commented 9 years ago

That is the same approach I took, to throw an exception if BOTH is specified. If you feel you wouldnt throw an exception how would you handle it instead?

On Sun, Jun 14, 2015 at 4:09 PM, mohataher notifications@github.com wrote:

That's a good question. I checked Frames and they throw any exception as well. Have a look here.

https://github.com/tinkerpop/frames/blob/master/src/main/java/com/tinkerpop/frames/annotations/IncidenceAnnotationHandler.java#L42 Honestly, I wouldn't throw any exceptions but document this to ensure the developer knows what he's doing. But if we're working towards rebuilding Frames, so following their strategy would be better.

— Reply to this email directly or view it on GitHub https://github.com/Syncleus/Ferma/pull/1#issuecomment-111872230.

mohataher commented 9 years ago

Ahaa, good you took the same approach. I think it's safer than my first approach.

I would return the last edge and write in the documentation that if you're using Direction.BOTH, two edges will be created but we will return the out edge. Would that cause any issues on the long term?

freemo commented 9 years ago

I dont like that solution as much, as it might lead people to make errors and not know about it, If they want to create both edges, but dont actually care nor need to have them both returned then they can implement it as an @Adjacency without any return type (in which case its functionally equivalent).

On Sun, Jun 14, 2015 at 4:14 PM, mohataher notifications@github.com wrote:

Ahaa, good you took the same approach. I think it's safer than my first approach.

I would send the last edge and write in the documentation that if you're using Direction.BOTH, two edges will be created but we will return the out edge. Would that cause any issues on the long term?

— Reply to this email directly or view it on GitHub https://github.com/Syncleus/Ferma/pull/1#issuecomment-111872431.

freemo commented 9 years ago

I just released version 2.0.6 of Ferma, it now includes these new features.

On Sun, Jun 14, 2015 at 4:16 PM, Jeffrey Freeman freemo@gmail.com wrote:

I dont like that solution as much, as it might lead people to make errors and not know about it, If they want to create both edges, but dont actually care nor need to have them both returned then they can implement it as an @Adjacency without any return type (in which case its functionally equivalent).

On Sun, Jun 14, 2015 at 4:14 PM, mohataher notifications@github.com wrote:

Ahaa, good you took the same approach. I think it's safer than my first approach.

I would send the last edge and write in the documentation that if you're using Direction.BOTH, two edges will be created but we will return the out edge. Would that cause any issues on the long term?

— Reply to this email directly or view it on GitHub https://github.com/Syncleus/Ferma/pull/1#issuecomment-111872431.