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

Specific structural types #42

Merged
merged 9 commits into from
Apr 25, 2022

Conversation

wgevaert
Copy link
Collaborator

@wgevaert wgevaert commented Apr 25, 2022

EDIT: This pr does the following:

  • Add the HasRelationshipsType and HasPropertiesType interfaces that extend the StructuralType interface.
  • Change the typehints used by DeleteClause to be only variables, similar to how the documentation of CYPHER writes delete clauses. (Note that DELETE (n:LABEL {property: 'value'}) etc. do not work, only DELETE n or DELETE (n), where the latter is functionally the same as the former.)
  • Change some typehints of RelationshipType and NodeType to be more specific.
  • Change some typehints that used to be StructuralType to be more specific.
  • Changes to Variable as discussed below
  • Remove unused imports
  • Sort imports
  • Add commas to last entries of multiline arrays
  • Add whitespace
    END OF EDIT

Make more specific structural types (HasPropertiesType and HasRelationsType) and let all three structural types implement StructuralType.

Also some codestyle fixes.

I also removed the structural types from Variable because I thought these did not make much sense. In my opinion, a variable can be the name of a structural type, but it is not itself a structural type. Maybe this does break some stuff of @transistive ? I am open for suggestions (as always).

@transistive
Copy link
Collaborator

I also removed the structural types from Variable because I thought these did not make much sense. In my opinion, a variable can be the name of a structural type, but it is not itself a structural type. Maybe this does break some stuff of @transistive ? I am open for suggestions (as always).

It does break some stuff.

How will you build this example?

MATCH (x:Node)
WITH x
MATCH (x) - [] -> (y:Node)

RETURN x, y

Do we have to build a new Query node with the name of the variable but no label instead of just passing the variable? That seems counterintuitive to me.

Also, why should the StructuralType be handled differently from a MapType? You can request properties on a name, but not a relationship? Continuing this logic in PHP userland, instanceof on a variable should not work as well, as it is always just a name of an entity and not the entity itself. It's an interesting philosophical discussion which I haven't thought about, but I don't think the case is as clear-cut as simply removing the StructuralType.

In my opinion as an end user, one of the main advantages of the query builder is reuse of common objects and patterns between clauses instead of having to keep track of the details such as names etc ourselves. That advantage is now lost for Variables and Structural types.

@wgevaert
Copy link
Collaborator Author

wgevaert commented Apr 25, 2022

You make a good point, I never thought of passing variables as nodes. I indeed always made new nodes instead of using the variables.

I think something like

class Variable {
    protected/private function toNode(): Node {
        return Query::node()->named($this);
    }

    public function relationship(....) {
        return $this->toNode()->relationship(...);
    }
    /*....*/
    withProperties(...) {
        return $this->toNode()->withProperties(...);
    }
    /*etc*/
}

Do you agree?

@transistive
Copy link
Collaborator

That sounds like a great compromise to me!

Thanks

Copy link
Collaborator

@marijnvanwezel marijnvanwezel left a comment

Choose a reason for hiding this comment

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

I see nothing strange, but I think it would be good if @transistive also looked at it.

How should be release this version? There are some very minor BC-breaks, but do they warrant a MAJOR release?

@wgevaert
Copy link
Collaborator Author

I believe the only BC break that might hinder end-users is the renaming DeleteClause::addNode -> addVariable.

All other changes only break things that users should not do in the first place.

See also this funny comic

Copy link
Collaborator

@transistive transistive left a comment

Choose a reason for hiding this comment

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

Looks great to me! 🥳

@wgevaert wgevaert merged commit 099aafa into neo4j-php:main Apr 25, 2022
@wgevaert wgevaert deleted the specific-structural-types branch April 25, 2022 14:24
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.

3 participants