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

A suggestion on NodeMixin interface #20

Merged
merged 1 commit into from Apr 27, 2011
Merged

Conversation

junegunn
Copy link
Contributor

Hi,
I think it would be more convenient if two NodeMixin class methods, property and index take arbitrary number of symbols followed by an optional hash.
Then we could write as follows,

  class Person
    include Neo4j::NodeMixin

    property :name, :address, :email, :type => String
    property :height, :weight, :type => Float
    index :name, :address, :type => :fulltext
    index :height, :weight
  end

This should feel natural to most Ruby developers.
I made changes to the both methods and spec files were extended accordingly.
The change is backward compatible.
So, what do you think? Please pull this patch if you think it's appropriate.
Thanks!

… symbols optionally followed by options hash
@andreasronge
Copy link
Member

Looks good. Does it works for Neo4j::Rails::Model and Neo4j::Rails::Relationship as well ?
Check https://github.com/junegunn/neo4j/blob/master/lib/neo4j/property/class_methods.rb

@junegunn
Copy link
Contributor Author

Yeah, it also works for them. I've tested, although I haven't extended the spec file. (my bad)

However, in fact, those two classes override the implementation of property method
by including Neo4j::Rails::Mapping::Property. And that version of property already processes arbitrary number of symbols and the options hash.
https://github.com/junegunn/neo4j/blob/master/lib/neo4j/rails/mapping/property.rb

So, this patch has no effect on property for those classes, but it still extends index.

andreasronge added a commit that referenced this pull request Apr 27, 2011
A suggestion on NodeMixin interface
@andreasronge andreasronge merged commit 6ad9f01 into neo4jrb:master Apr 27, 2011
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

2 participants