-
Notifications
You must be signed in to change notification settings - Fork 46
Unique attribute ownership #266
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
PR Review ChecklistDo not edit the content of this comment. The PR reviewer should simply update this comment by ticking each review item below, as they get completed. Trivial Change
Code
Architecture
|
grammar/TypeQL.g4
Outdated
//annotations : ( ANNOTATION_KEY )? ( ANNOTATION_UNIQUE )? ( ANNOTATION_RANGE | ANNOTATION_REGEX )? ; | ||
|
||
owns_annotations : ( ANNOTATION_KEY )? ( ANNOTATION_UNIQUE )? ; |
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.
@haikalpribadi do you think we should use a generic annotations
block that can be any annotations, and then do the validation once we construct the TypeQL objects (see the commented annotations
above, which we would apply using:
as OWNS type (as type)? annotations
In the future we will have long list of annotations (range, cardinality, regex, etc.) that are possible, but only some will be applicable to owns
, some will be applicable to plays
only, some will be applicable to the variable directly... given this I think we should have owns_annotations
separate, then add over time.
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.
What if we can provide more than one annotation? 🤔
Also, I think the it should be annotation_own
and annotation_relation
, no?
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.
Yeah i think annotation_owns
makes more sense - was thinking that grouping it as "owns" first was easier to read but actually is less consistent.
I think it would be on the constraint name eg. annotation_owns
and annotation_relates
rather than suggested annotation_own
and annotation_relation
though.
The proposed mechanism does allow us to provide more than one annotation, for example @unique @cardinality(...)
would be fine. The alternative is inline:
OWNS type ( AS type )? ( ANNOTATION_KEY )? ( ANNOTATION_UNIQUE )?
which also works but will overflow the line and isn't as nice to read.
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.
Why the overflow? You mean the line becomes too long horizontally?
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
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.
why not wrap it in another rule? like what you had before but better named?
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.
you mean the entire row for type_owns_constraint
, or just keep the OWNS type
as is and add a new annotations_owns = ANNOTATION_KEY )? ( ANNOTATION_UNIQUE )?
similar to what I suggested?
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.
(annotations_owns = ANNOTATION_KEY )? ( ANNOTATION_UNIQUE )?
enforces an order on the annotations
@key @unique
parses, whereas @unique @key
does not.
public Set<TypeQLToken.Annotation> visitOwns_annotations(TypeQLParser.Owns_annotationsContext ctx) { | ||
Set<TypeQLToken.Annotation> annotations = new HashSet<>(); | ||
if (ctx.ANNOTATION_KEY() != null) annotations.add(parseAnnotation(ctx.ANNOTATION_KEY())); | ||
else if (ctx.ANNOTATION_UNIQUE() != null) annotations.add(parseAnnotation(ctx.ANNOTATION_UNIQUE())); |
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.
In the case where both @key
and @unique
are present, this will silently ignore the @unique
. This results in the right behaviour, but isn't faithful to what's been written
Replaced by #273 |
For the title of this PR: please follow the grammatical rules of a usual publication title, without capitalisation (except for the first letter). Thus, the title should NOT CONTAIN CODE: no dots, no parentheses, no backticks, no brackets, etc. It needs to be distinctive (not detailed) and succinct (not lengthy). Details of this PR will go in the description. For the description of this PR: please replace every line in curly brackets ( { like this } ) with an appropriate description following the guidance. Finally, please remove this paragraph.
What is the goal of this PR?
{ In the form of a paragraph (only use bullet points if strictly necessary), please describe the goal of this PR, why they are valuable to achieve, and reference the related GitHub issues. This section will be automatically compiled into the release notes, so please:
What are the changes implemented in this PR?
{ Please explain what you implemented, why your changes are the best way to achieve the goal(s) above. Please describe every method, class and package, by explaining:
This would allow the reviewer to understand your intentions in the code much better. If you're adding new classes, make sure these explanations are also included in the class header comments. Last but not least, please reference the GitHub issues to be automatically closed, such like 'closes #number'. }