-
Notifications
You must be signed in to change notification settings - Fork 59
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
fix: Parsing for qualifiers with colon characters #1277
Conversation
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
Please fix the tests |
Warning: This pull request is touching the following templated files:
|
All done, tested locally. Apologies for the noise. Changed the qualifier assertion to check for |
src/mutation.ts
Outdated
// columnName does not contain ':' | ||
return { | ||
family: columnName, | ||
qualifier: null, |
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 recommend setting qualifier
to undefined
so that this code works the same way as before when the string doesn't contain a colon.
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 tried that too, however the ParsedColumn
interface does not declare undefined
as an acceptable type for qualifier
at https://github.com/googleapis/nodejs-bigtable/blob/main/src/mutation.ts#L35.
Adding undefined
here would solve this:
export interface ParsedColumn {
family: string | null;
qualifier: string | null | undefined;
}
Consider adding another test? |
parseColumnName()
uses.split(':')
to find the qualifier, but this parses qualifiers with:
chars in it incorrectly, since they also are picked up by the split. This causes it to ignore everything after:
in the qualifier.e.g. consider a column name
"foo:bar:baz"
parseColumnName()
parses this incorrectly asUsing
slice()
instead with the index of the first:
character fixes this: