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

Adds errors for non-declared procedures #245

Merged
merged 20 commits into from
Sep 18, 2024
Merged

Conversation

angrykoala
Copy link
Member

@angrykoala angrykoala commented Jul 18, 2024

Extends #234 with the same warnings errors for procedures

@angrykoala angrykoala requested a review from ncordon July 18, 2024 12:39
Copy link

changeset-bot bot commented Jul 18, 2024

🦋 Changeset detected

Latest commit: 30ddb3f

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

Base automatically changed from database-functions-warning to main July 18, 2024 15:37
@ncordon ncordon marked this pull request as ready for review September 5, 2024 09:46
@ncordon ncordon changed the title Procedure not found warning Adds errors for non-declared procedures Sep 5, 2024
@@ -39,11 +40,12 @@ export interface ParsedStatement {
// A statement needs to be parsed with the .statements() rule because
// it's the one that tries to parse until the EOF
ctx: StatementsOrCommandsContext;
diagnostics: SyntaxDiagnostic[];
syntaxErrors: SyntaxDiagnostic[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

why rename this? I think even if "syntaxErrors" could be more desriptive, it's nice to match the type name imo

Copy link
Collaborator

Choose a reason for hiding this comment

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

It makes the code more readable in my opinion. Especially when we are using web workers and we need that logic of: no syntax errors, then execute the web worker. It reads better than diagnostics.

Another example is what has changed in syntaxValidation.ts:

    const anySyntacticError =
      statements.filter((statement) => statement.syntaxErrors.length !== 0)
        .length > 0;

We know what's happening inside better than if it read diagnostics.

What we could do is rename this type:

export type SyntaxDiagnostic = Diagnostic & {
  offsets: { start: number; end: number };
};

where Diagnostic is coming from the vscode-types. Any suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't feel very strongly about it & agree syntaxError reads better. Part of me was assuming there could be more than errors from a "Diagnostics" though

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's beside the point but for the code example you could use [some](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/some) instead, it's more efficient as it stops iterating as soon as it finds a match

const anySyntacticError =
      statements.some((statement) => statement.syntaxErrors.length !== 0)

@@ -38,7 +38,7 @@
"name": "keyword.operator"
},
{
"match": "(?i)\\b(ACCESS|ACTIVE|ADMIN|ADMINISTRATOR|ALIAS|ALIASES|ALL|allShortestPaths|ALTER|AND|ANY|ARRAY|AS|ASC|ASCENDING|ASSERT|ASSIGN|AT|AUTH|BINDINGS|BOOL|BOOLEAN|BOOSTED|BOTH|BREAK|BRIEF|BTREE|BUILT|BY|CALL|CASE|CIDR|CHANGE|COLLECT|COMMAND|COMMANDS|COMMIT|COMPOSITE|CONSTRAINT|CONSTRAINTS|CONTAINS|CONTINUE|COPY|COUNT|CREATE|CSV|CONCURRENT|CURRENT|DATA|DATABASE|DATABASES|DATE|DATETIME|DBMS|DEALLOCATE|DEFAULT|DEFINED|DELETE|DENY|DESC|DESCENDING|DESTROY|DETACH|DIFFERENT|DISTINCT|DRIVER|DROP|DRYRUN|DUMP|DURATION|EACH|EDGE|ELEMENT|ELEMENTS|ELSE|ENABLE|ENCRYPTED|END|ENDS|ERROR|EXECUTABLE|EXECUTE|EXIST|EXISTENCE|EXISTS|FAIL|FALSE|FIELDTERMINATOR|FINISH|FLOAT|FOR|FOREACH|FROM|FULLTEXT|FUNCTION|FUNCTIONS|GRANT|GRAPH|GRAPHS|GROUP|GROUPS|HEADERS|HOME|ID|IF|IMMUTABLE|IMPERSONATE|IN|INDEX|INDEXES|INF|INFINITY|INSERT|INT|INTEGER|IS|JOIN|KEY|LABEL|LABELS|LEADING|LIMIT|LIST|LOAD|LOCAL|LOOKUP|MANAGEMENT|MAP|MATCH|MERGE|NAME|NAMES|NAN|NEW|NFC|NFD|NFKC|NFKD|NODE|NODETACH|NODES|NONE|NORMALIZE|NORMALIZED|NOT|NOTHING|NOWAIT|NULL|OF|ON|ONLY|OPTION|OPTIONAL|OPTIONS|OR|ORDER|OUTPUT|PASSWORD|PASSWORDS|PATH|PATHS|PERIODIC|PLAINTEXT|POINT|POPULATED|PRIMARY|PRIMARIES|PRIVILEGE|PRIVILEGES|PROCEDURE|PROCEDURES|PROPERTIES|PROPERTY|PROVIDER|PROVIDERS|RANGE|READ|REALLOCATE|REDUCE|REL|RELATIONSHIP|RELATIONSHIPS|REMOVE|RENAME|REPEATABLE|REPLACE|REPORT|REQUIRE|REQUIRED|RETURN|REVOKE|ROLE|ROLES|ROW|ROWS|SCAN|SECONDARY|SECONDARIES|SEC|SECOND|SECONDS|SEEK|SERVER|SERVERS|SET|SETTING|SETTINGS|SHORTEST|shortestPath|SHOW|SIGNED|SINGLE|SKIP|START|STARTS|STATUS|STOP|VARCHAR|STRING|SUPPORTED|SUSPENDED|TARGET|TERMINATE|TEXT|THEN|TIME|TIMESTAMP|TIMEZONE|TO|TOPOLOGY|TRAILING|TRANSACTION|TRANSACTIONS|TRAVERSE|TRIM|TRUE|TYPE|TYPED|TYPES|UNION|UNIQUE|UNIQUENESS|UNWIND|URL|USE|USER|USERS|USING|VALUE|VECTOR|VERBOSE|VERTEX|WAIT|WHEN|WHERE|WITH|WITHOUT|WRITE|XOR|YIELD|ZONED|EXPLAIN|PROFILE)\\b",
"match": "(?i)\\b(ACCESS|ACTIVE|ADMIN|ADMINISTRATOR|ALIAS|ALIASES|ALL|allShortestPaths|ALTER|AND|ANY|ARRAY|AS|ASC|ASCENDING|ASSERT|ASSIGN|AT|AUTH|BINDINGS|BOOL|BOOLEAN|BOOSTED|BOTH|BREAK|BRIEF|BTREE|BUILT|BY|CALL|CASE|CIDR|CHANGE|COLLECT|COMMAND|COMMANDS|COMMIT|COMPOSITE|CONSTRAINT|CONSTRAINTS|CONTAINS|CONTINUE|COPY|COUNT|CREATE|CSV|CONCURRENT|CURRENT|DATA|DATABASE|DATABASES|DATE|DATETIME|DBMS|DEALLOCATE|DEFAULT|DEFINED|DELETE|DENY|DESC|DESCENDING|DESTROY|DETACH|DIFFERENT|DISTINCT|DRIVER|DROP|DRYRUN|DUMP|DURATION|EACH|EDGE|ELEMENT|ELEMENTS|ELSE|ENABLE|ENCRYPTED|END|ENDS|ERROR|EXECUTABLE|EXECUTE|EXIST|EXISTENCE|EXISTS|FAIL|FALSE|FIELDTERMINATOR|FINISH|FLOAT|FOR|FOREACH|FROM|FULLTEXT|FUNCTION|FUNCTIONS|GRANT|GRAPH|GRAPHS|GROUP|GROUPS|HEADERS|HOME|ID|IF|IMMUTABLE|IMPERSONATE|IN|INDEX|INDEXES|INF|INFINITY|INSERT|INT|INTEGER|IS|JOIN|KEY|LABEL|LABELS|LEADING|LIMIT|LIST|LOAD|LOCAL|LOOKUP|MANAGEMENT|MAP|MATCH|MERGE|NAME|NAMES|NAN|NEW|NFC|NFD|NFKC|NFKD|NODE|NODETACH|NODES|NONE|NORMALIZE|NORMALIZED|NOT|NOTHING|NOWAIT|NULL|OF|ON|ONLY|OPTION|OPTIONAL|OPTIONS|OR|ORDER|OUTPUT|PASSWORD|PASSWORDS|PATH|PATHS|PERIODIC|PLAINTEXT|POINT|POPULATED|PRIMARY|PRIMARIES|PRIVILEGE|PRIVILEGES|PROCEDURE|PROCEDURES|PROPERTIES|PROPERTY|PROVIDER|PROVIDERS|RANGE|READ|REALLOCATE|REDUCE|REL|RELATIONSHIP|RELATIONSHIPS|REMOVE|RENAME|REPEATABLE|REPLACE|REPORT|REQUIRE|REQUIRED|RETURN|REVOKE|ROLE|ROLES|ROW|ROWS|SCAN|SECONDARY|SECONDARIES|SEC|SECOND|SECONDS|SEEK|SERVER|SERVERS|SET|SETTING|SETTINGS|SHORTEST|shortestPath|SHOW|SIGNED|SINGLE|SKIP|START|STARTS|STATUS|STOP|VARCHAR|STRING|SUPPORTED|SUSPENDED|TARGET|TERMINATE|TEXT|THEN|TIME|TIMESTAMP|TIMEZONE|TO|TOPOLOGY|TRAILING|TRANSACTION|TRANSACTIONS|TRAVERSE|TRIM|TRUE|TYPE|TYPED|TYPES|UNION|UNIQUE|UNIQUENESS|UNWIND|URL|USE|USER|USERS|USING|VALUE|VECTOR|VERBOSE|VERTEX|WAIT|WHEN|WHERE|WITH|WITHOUT|WRITE|XOR|YIELD|ZONE|ZONED|EXPLAIN|PROFILE)\\b",
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's new here? It's hard to see from the git change what's updated

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's just the ZONE keyword, but we haven't touched anything in this pr. I think it's because we don't have a CI job making sure that grammar has been regenerated when we make changes.

Copy link
Collaborator

@OskarDamkjaer OskarDamkjaer left a comment

Choose a reason for hiding this comment

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

Only got some code style/structure remarks, impl is good to go from my point over view 👍

@ncordon ncordon merged commit 2e72ac8 into main Sep 18, 2024
3 checks passed
@ncordon ncordon deleted the database-procedures-warning branch September 18, 2024 08:25
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