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

feat: basic Redis grammar, parseRedisQueryWithoutCursor function, tests #207

Merged
merged 9 commits into from
Aug 7, 2024

Conversation

roberthovsepyan
Copy link
Contributor

Closes #ISSUE_ID

});

test('should not report errors on multiple INCR commands', () => {
const autocompleteResult = parseRedisQueryWithoutCursor('INCR test\nINCR 123');
Copy link
Collaborator

@NikitaShkaruba NikitaShkaruba Aug 7, 2024

Choose a reason for hiding this comment

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

We split queries by newline? Maybe it's better to force ; in the end? This way it's gonna be easier for us to parse multiple commands too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can't use semicolon, because it is valid to use in key/value name, e.g. SET key value; will set value to value;
that's why Redis commands are separated by newline, that's also the way they do it in DataGrip

| command NEWLINE commands
;

command
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure we should call it command, and not statement? Statements is a common name afaik

Copy link
Contributor Author

@roberthovsepyan roberthovsepyan Aug 7, 2024

Choose a reason for hiding this comment

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

that's the way they call it everywhere in regards to Redis (including official docs), so I thought we better stick to conventional naming
Screenshot 2024-08-07 at 17 06 51

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice one!

const autocompleteResult = parseRedisQueryWithoutCursor('GET test\nGET 123');

expect(autocompleteResult.errors).toHaveLength(0);
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's better to have a separete multiple commands test. Because in the end we want to test commands rule 3fd0a09#diff-b96534631ee82ffc85910f193fcacb74b496e3324eef92e1d7121103f2321683R16

});

test('should not report errors on SET command with all argument', () => {
const autocompleteResult = parseRedisQueryWithoutCursor('SET test test XX GET PXAT 123');
Copy link
Collaborator

Choose a reason for hiding this comment

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

These XX, EXAT are declared in the redis grammar? Or is it our doing? How will we document this syntax in our documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's part of official syntax of course - https://redis.io/docs/latest/commands/set/
I don't see why we need to document Redis syntax in our own docs

;

setCommand
: SET keyName identifier (NX | XX)? GET? expirationClause?
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is NX | XX? Let's move it to a separate rule for improved reliability?

Copy link
Collaborator

@NikitaShkaruba NikitaShkaruba left a comment

Choose a reason for hiding this comment

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

Nice job!

@roberthovsepyan roberthovsepyan merged commit 584f902 into main Aug 7, 2024
4 checks passed
@roberthovsepyan roberthovsepyan deleted the feat/redis-init branch August 7, 2024 14:31
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