Fixes backticking of params #457
Conversation
…ngle one + testing
🦋 Changeset detectedLatest commit: ece928e The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
| if (e == null || e == '') { | ||
| return undefined; | ||
| } else if (/[^\p{L}\p{N}_]/u.test(e) || /[^\p{L}_]/u.test(e[0])) { | ||
| return `\`${e}\``; | ||
| } else { | ||
| return undefined; | ||
| } | ||
| } | ||
| type BacktickVariant = 'label' | 'propertyKey' | 'relType' | 'dbName' | 'param'; |
There was a problem hiding this comment.
I don't see the need for having these many types. The only one that is different is param, so having 'param' | 'nonParam' would be more concise
There was a problem hiding this comment.
dbName also has a special rule (periods are allowed without backticks if it's not the first or last symbol), and I figured for readability it's nice to see what type of elements we use which regex for
I did notice however that the old method was not checking for a period in the end of the dbname, so will adjust that
| (/[^\p{L}\p{N}_.]/u.test(e) || | ||
| /[^\p{L}_]/u.test(e[0]) || | ||
| /[^\p{L}\p{N}_]/u.test(e[e.length - 1])) |
There was a problem hiding this comment.
The _ is not valid in a database name.
The nicer Typescript way to get the last element is e.at(-1)
There was a problem hiding this comment.
If you are going to change this you should include tests for some db names:
some.database
database-
database.
100database
There was a problem hiding this comment.
yeah I noticed the db-name thing while testing the last letter. Since this means that dbs cant have a _ in the string, e will never contain _, so this check doesnt affect valid dbnames. And since it still works for both direct names and aliases (and I think dbName is a nice shared variant name) I figure we can avoid an extra case by keeping it as is
There was a problem hiding this comment.
e2e was failing so I think maybe some small detail was broken at the same time, but ill adjust to the nicer "-1"-way of indexing when i get to the bottom of the e2e test (edit: it seems like it was just flaky)
There was a problem hiding this comment.
Didnt see your second comment on my last message(s), but yeah we dont have any tests for certain odd but possible endings like - and ., so I will add these
Before we used the same backticking condition as the one for labels/reltypes/propertyKeys, which cant start with a number. Params can though, so we need a different condition.
Ex.