philippkueng / node-neo4j

Neo4j REST API wrapper for Node.js
MIT License
211 stars 44 forks source link

readAllRelationshipsOfNode – can I specify the type of relationship? #36

Closed deemeetree closed 10 years ago

deemeetree commented 10 years ago

Is it possible to specify the type of relationships that need to be queried using db.readAllRelationshipsOfNode or do I need to use db.cypherQuery for that?

Thank you!

freeeve commented 10 years ago

I looked briefly but couldn't find an implementation for that particular request (http://docs.neo4j.org/chunked/milestone/rest-api-relationships.html#rest-api-get-typed-relationships)

Shouldn't be too hard to add, though... very similar to the existing ones, with an added type.

philippkueng commented 10 years ago

Thanks @wfreeman, you're fast. Seems indeed to be fairly trivial. @deemeetree could you create a PR? otherwise i can make that change too, but on friday the earliest.

deemeetree commented 10 years ago

@wfreeman @philippkueng Thank you for the fast response! I'll look in the code and it will be great if we can also add the same functionality for reading directed relationships as well, such as readIncomingRelationshipsOfNode and readOutgoingRelationshipsOfNode

freeeve commented 10 years ago

Yeah, similar. What do you guys think the names should be? readIncomingTypedRelationshipsOfNode? readTypedIncomingRelationshipsOfNode?

Probably should be refactored to reuse the same function at this point, since there's a lot of repetition between the different versions.

deemeetree commented 10 years ago

I think readIncomingTypedRelationshipsOfNode following the convention there already is and it's more readable like that. But also why not just add a second optional parameter, which would specify the type to avoid creating too many functions? Probably also could be good if several types could be listed at once, but I don't know how difficult that.

philippkueng commented 10 years ago

Hi @deemeetree and @wfreeman,

Thanks for your input and thinking. Also sorry for the late response. I think going with an optional parameter might be the smartest move here in order not to create more code bloat and also keep the amount of different functions fairly small.

Thinking about a function definition like this Neo4j.prototype.readAllRelationshipsOfNode = function(node_id, [options], callback){}

With options having the form:

{
  'types': ['type1', 'type2', ...]
}

Would that be ok for you both? It has the added benefit that other options could be added easily and it doesn't break existing implementations.

freeeve commented 10 years ago

I don't think the name readAll... makes sense when you're only reading part of them! :) Maybe just readRelationshipsOfNode? or even more concise: getRels()?... although it doesn't match the rest of the API.

philippkueng commented 10 years ago

Good point, thank you. Keeping the function readAll... for legacy and introducing readRelationshipsOfNode. Think getRels on the other hand might promise too much in one function.

freeeve commented 10 years ago

sounds good.

philippkueng commented 10 years ago

Hi @wfreeman and @deemeetree,

Ok, the tagged relationships are implemented, documented (https://github.com/philippkueng/node-neo4j#advanced-relationship-operations) and released in 2.0.0-RC5. Let me know if you're missing anything else.

Best, Philipp