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

Extended parsing of preparser options #301

Merged
merged 13 commits into from
Dec 18, 2024
Merged

Conversation

anderson4j
Copy link
Collaborator

added parsing of preparser options for cypher versions and key=value
Can now parse queries like CYPHER <versionnumber> <option1> = <value1> <option2> = <value2>
where options are identifiers and values either identifiers or numberLiterals

Copy link

changeset-bot bot commented Nov 28, 2024

🦋 Changeset detected

Latest commit: 4ff7cf0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@neo4j-cypher/language-support Patch
@neo4j-cypher/language-server Patch
@neo4j-cypher/react-codemirror-playground Patch
@neo4j-cypher/react-codemirror Patch
@neo4j-cypher/schema-poller Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@anderson4j
Copy link
Collaborator Author

anderson4j commented Nov 28, 2024

I guess this should have a changeset but no version bumps?
Edit: added as mentioned above

@anderson4j anderson4j requested a review from ncordon November 28, 2024 14:55
@anderson4j anderson4j force-pushed the extendedPreparserOptions branch from 9c8384a to 6d736dc Compare December 4, 2024 15:12
EXPLAIN | PROFILE | CYPHER (UNSIGNED_DECIMAL_INTEGER | IDENTIFIER EQ (IDENTIFIER | numberLiteral))*;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be cleaner to extract the Cypher part to another rule?

preparserOption:
  EXPLAIN | PROFILE | cypherOptions

cypherOptions:
   CYPHER (cypherVersion | cypherOption)*

Copy link
Collaborator

@ncordon ncordon left a comment

Choose a reason for hiding this comment

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

We should add some syntax colouring tests.

Also there's a test in syntacticValidation.test.ts that checks we can use preparser keywords in other parts of the code and it still works, so we should have something for CYPHER there.

There's a rule in CypherCmdParser that overrides the keywords we can use:

// This rule overrides the identifiers adding EXPLAIN, PROFILE, etc
unescapedLabelSymbolicNameString: 
    preparserOption 
    | HISTORY
    | CLEAR
    | PARAM
    | CONNECT
    | DISCONNECT
    | WELCOME
    | SYSINFO
    | unescapedLabelSymbolicNameString_
    ;

Probably best to add a preparserKeyword: EXPLAIN | PROFILE | CYPHER to the preparser so we can rely on that one.

@anderson4j
Copy link
Collaborator Author

Don't think I understand the comment about preparserKeyword. You want preparserKeyword clashing with preparserOption?

@anderson4j
Copy link
Collaborator Author

Here's a comparison query for the new colors
Before:
image
After:
image

@anderson4j
Copy link
Collaborator Author

Also in the background the tokentype for the setting name ("runtime" here) was changed, but seems vscode currently has the same color for the new tokentype as the old. Considered removing it, but I suppose the different grouping would have an effect if vscode changes coloring

@ncordon
Copy link
Collaborator

ncordon commented Dec 12, 2024

Probably assigning enum and enumMember to the key and value would give more contrast difference between the colours.

@anderson4j
Copy link
Collaborator Author

anderson4j commented Dec 16, 2024

Yup that looks nicer. TODO is still test making (Edit: not todo anymore)

@anderson4j anderson4j merged commit 84a12fc into main Dec 18, 2024
4 checks passed
@anderson4j anderson4j deleted the extendedPreparserOptions branch December 18, 2024 10:05
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.

2 participants