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.

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

Release 3.0.0.alpha.7 (the relationships edition) #353

Closed
wants to merge 1 commit into from
Closed

Release 3.0.0.alpha.7 (the relationships edition) #353

wants to merge 1 commit into from

Conversation

anulman
Copy link

@anulman anulman commented Jun 6, 2014

  • Refactor HasN::DeclRel syntax to better match ActiveRecord
  • Add build/where functions on DeclRel collections
  • Add HasN::Rels class
  • ActiveNode#where, ActiveNode#first helpers
  • Introducing ActiveRel

 - Refactor HasN::DeclRel syntax to better match ActiveRecord
 - Add build/where functions on DeclRel collections
 - Add HasN::Rels class
 - ActiveNode#where, ActiveNode#first helpers
 - Introducing ActiveRel
@anulman
Copy link
Author

anulman commented Jun 6, 2014

@andreasronge—Sorry this PR doesn't yet pass the spec suite.

The gem is now working as I would expect it to in a sample app (n.b. with the new has_n syntax). Do you have any feedback on the changes and/or implementation? I'd rather not waste cycles on conformance for conformance's sake if at all possible :)

@@ -33,7 +33,7 @@ It comes included with the Apache Lucene document database.
s.add_dependency("activemodel", "~> 4")
s.add_dependency("railties", "~> 4")
s.add_dependency('active_attr', "~> 0.8")
s.add_dependency("neo4j-core", "= 3.0.0.alpha.14")
s.add_dependency("neo4j-core", "= 3.0.0.alpha.15")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the reason why travis does not build. Have you changed neo4j-core as well ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. It's a dependency, as listed :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, sorry did not see the pull request on neo4j-core.

@andreasronge
Copy link
Member

This contains really nice stuff. However, I would like to fix the following before merging it

  • Add Rspecs
  • Fix code duplication (as you have mentioned in comments) between ActiveNode and ActiveRel
  • Remove the cypher queries (where), and wait for crel to be implemented, or use the Neo4j::Session.query instead of generating your own cypher strings.

@anulman
Copy link
Author

anulman commented Jun 9, 2014

Thanks Andreas :)

Sounds like a plan! I should have #s 1 and 2 implemented fairly quickly; however, I wasn't able to get Neo4j::Session.query working without a ton of code duplication due to relationship directionality and wonky match requirements.

What's left before crel is implemented? I see you and @cheerfulstoic discussing API documentation in the other thread; is that it?

@cheerfulstoic
Copy link
Contributor

I'm currently boning up on my cypher knowledge and re-thinking it somewhat from the ground up to move it from neo4j to neo4j-core. Currently it's more in the model world and I think it needs to be strictly in the query-generation world, so it's going to need to work differently for that.

I'm putting together a draft of some wiki docs which we can look over. Should be ready in a day or two.

@anulman
Copy link
Author

anulman commented Jun 9, 2014

@cheerfulstoic I may have built a clunky version of this in CypherQuery (FILE from my neo4j-core PR): cypher_query consists of two arrays—nodes and rels—of QueryElements (conditions); cypher_query.to_s composes its clauses by iterating over cypher_query.elements.

I intended to abstract the condition templates (currently instance methods in QueryElements) to a design like the one you expressed wonderfully in crel. If the CypherQuery code/design has any place in what you're working on, perhaps we could work together.

@cheerfulstoic
Copy link
Contributor

Honestly I'm open to different ideas right now ;) I'd like to evaluate ASTs as a possible solution and I'm currently looking into arel's implementation. I don't want to do it wrong, but at the same time I don't want to sit around and theorize about it forever either ;)

I've created a new thread for crel discussion in the neo4j-core gem repo over here, BTW: neo4jrb/neo4j-core#75

@andreasronge
Copy link
Member

Hope you don't mind if I close this since it is imp. in #393 #411

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants