Optional autocomplete of relationships

[HEnquist]

This is a simple implementation for autocompleting relationships in the graph. Solves #1, but it's a simple on/off switch so it does not address the part about providing a list of relationship types.

[yGuy]

Interesting, thanks so much for your contribution!

The fact that this depends on apoc is a bit sad. I don't really understand, why this is necessary, though, to be honest.

@fskpf - I think the implemenation that we are using in App Generator uses a much simpler cypher query to get the additional information and it does not depend on apoc. Can you check out our solution and see if we can merge the ideas. I think doing the filtering to relationship types should be done on the api level, too, before we merge this because otherwise this would be a slightly incompatible API change in the future. We can always just perform the check/filter in python, too, if the cypher query gets too complicated.

[HEnquist]

The fact that this depends on apoc is a bit sad. I don't really understand, why this is necessary, though, to be honest.

Not it's not necessary, the neo4j browser does the same thing without it. But it's very convenient, and apoc is often installed (just not always). My thinking was basically that this works now and could be useful for many others too. It's always possible to improve later.

[yGuy]

Couldn't we just use a cypher query like this?

https://github.com/yWorks/yfiles-for-html-demos/blob/7830f4df3ddd33e3517adb2546a00cf478e99b65/demos/toolkit/neo4j/Neo4jDemo.ts#L302

this would also trivially allow us to specify the types of relationships:

MATCH (n)-[rel]-(m)
            WHERE id(n) IN $ids
            AND id(m) IN $ids
            AND startNode(rel) <> endNode(rel)
            RETURN DISTINCT rel 

or

MATCH (n)-[rel:REL_TYPE1|REL_TYPE1|REL_TYPE2]-(m)
            WHERE id(n) IN $ids
            AND id(m) IN $ids
            AND startNode(rel) <> endNode(rel)
            RETURN DISTINCT rel 

we would then need to add these relationships to the ones returned by the original query, of course.

[HEnquist]

Yes this works, that was actually easier than I initially thought.
I have implemented this, but I left out AND startNode(rel) <> endNode(rel) because I want to also include relationships that start and end on the same node.

[yGuy]

Thanks! Great!
Is this behavior (showing self-loops) in line with what the neo4j browser does? To a degree I would understand if people would be surprised to see edges when all they do is query a single node. If neo4j does it differently, we should make this configurable, IMHO. Other than that, I hope that we can merge this, @fskpf !

[HEnquist]

Thanks! Great! Is this behavior (showing self-loops) in line with what the neo4j browser does? To a degree I would understand if people would be surprised to see edges when all they do is query a single node.

Yes the neo4j browser also behaves like this, self-loops are shown even if you just match a single node.

[fskpf]

My IDE complains about this and suggests simplifying it to if self._autocomplete_relationships:.
However, changing it to the simplified code breaks the logic because an array of rel-types would then return "" instead of the additional clause.

Can you refactor it to make the IDE happy (and avoid other devs just "simplifying" the code with ALT+ENTER and thus breaking it in the future)?

[HEnquist]

Please see the next comment!

[fskpf]

The IDE warns about the len(self._autocomplete_relationships) statements:

Expected type 'Sized', got 'bool' instead

Guess this is due to the dynamic type of the _autocomplete_relationships any chance of improving this?

[HEnquist]

I have updated this, and made it a bit simpler. I don't know what IDE you are using (and what tool that is using to analyze the code), but at least pylint is happy with the new version.

[fskpf]

@HEnquist thank you for the PR, I've added some notes, please have a second look. Other than the minor comments, it seems to work as intended.

Another question: What is the pythonic way of using the properties?

In my test case, I used

g._autocomplete_relationships = ["ACTED_IN"]

or

g._autocomplete_relationships = True

But just using g.autocomplete_relationships = True without the leading underscore didn't work. Using the underscore feels like setting a private property to me.

[HEnquist]

@HEnquist thank you for the PR, I've added some notes, please have a second look. Other than the minor comments, it seems to work as intended.

Another question: What is the pythonic way of using the properties?

In my test case, I used

g._autocomplete_relationships = ["ACTED_IN"]

or

g._autocomplete_relationships = True

The _autocomplete_relationships is a private property, you should normally not touch anything with a leading underscore. I implemented this as described in #1, with a setter method called set_autocomplete_relationships().

But just using g.autocomplete_relationships = True without the leading underscore didn't work. Using the underscore feels like setting a private property to me.

It would be no problem to switch from the setter method to a property (https://docs.python.org/3/library/functions.html#property), so you can do g.autocomplete_relationships = True instead of g.set_autocomplete_relationships(True). The driver property is accessed via a setter and getter, which is a good argument for staying with a setter here (or changing both to properties). Just let me know what you prefer.

[fskpf]

Thanks for the explanation. I'd say keep the setter method for now. We can still switch to properties in a future update then.

I'm not deep into Python, what would you say is more common for libraries? Actual properties or setter/getter methods? If there is a case to change it, we should create a different issue then.

[HEnquist]

Thanks for merging and for the great feedback along the say!

I'm not deep into Python, what would you say is more common for libraries? Actual properties or setter/getter methods? If there is a case to change it, we should create a different issue then.

Both ways are used, I couldn't say which one is the more common. I would say that properties are somewhat more pythonic. But it's questionable if that is reason enough to switch :D