-
Notifications
You must be signed in to change notification settings - Fork 132
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
Update constraint syntax #1238
Update constraint syntax #1238
Conversation
Should we add |
dd1263c
to
38da100
Compare
Co-authored-by: Therese Magnusson <[email protected]>
38da100
to
e09294f
Compare
CIP: opencypher/openCypher#166 (and just the updates we did https://docs.google.com/document/d/1LemTgk1hcWQhSlFl9jEsiKciYyBV2yf_XVDgKzt29RI/edit?usp=sharing I believe) Accompanying manual-modeling PR: https://github.com/neo-technology/neo4j-manual-modeling/pull/2332 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks good, only file I found which you should also consider updating is PrettifierTest.scala. And then I have some smaller thoughts I put in the comments.
I have built the Cypher manual and confirmed everything looks ok, not sure how to build the refcard these days though.
cypher/cypher-docs/src/docs/dev/deprecations-additions-and-compatibility.asciidoc
Outdated
Show resolved
Hide resolved
cypher/cypher-docs/src/test/scala/org/neo4j/cypher/docgen/ConstraintsTest.scala
Outdated
Show resolved
Hide resolved
cypher/cypher-docs/src/docs/dev/deprecations-additions-and-compatibility.asciidoc
Outdated
Show resolved
Hide resolved
There are three such files, but I think I remember something about those being unused so that's why we skipped them ( |
cypher/cypher-docs/src/test/scala/org/neo4j/cypher/docgen/ConstraintsTest.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the comment I left, I think this looks good. It's a fairly straightforward update. 👍
testQuery( | ||
title = "Create a unique constraint using deprecated syntax", | ||
text = "The unique constraint ensures that your database " + | ||
"will never contain more than one node with a specific label and one property value.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"will never contain more than one node with a specific label and one property value.", | |
"never contains more than one node with a specific label and one property value.", |
We like to use present tense and active voice, but I am aware that this has not been consistently implemented, and not in this section either. Perhaps it is better to leave it as is, for consistency's sake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this is just copy paste from another test in the same file, so perhaps it is better to make a cleanup card on either your Trello board our ours about updating it across the board.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, it's best dealt with separately. And with that, I have no more objections. Should I merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please do :)
Syntax changed to
FOR ... REQUIRE
instead ofON ... ASSERT