Skip to content
This repository was archived by the owner on Apr 14, 2022. It is now read-only.

graphql: implement c-style expressions parser #209

Merged

Conversation

Hollow111
Copy link
Contributor

Needed for #13

Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

Thanks for the patch!

I think it has quite good quality already, but left thoughts on possible improvements (some of them are just for consistency with the rest code base) and rice some questions. BTW, questions from a reviewer often are good candidates to describe things in a comment or a commit message.

Use branch inside the repository instead of a fork: it eases review of the changes locally (just git checkout).

It is good practice to add the prefix 'WIP: ' for pull requests in work to avoid misunderstanding of the status and prevent premature check-in.

Please, consider comments below. Consider they as recommendations that are debatable. Consider a test comments with lower priority then ones of the code.

@Hollow111 Hollow111 force-pushed the N_Tatunov/gh-13-c-style-expressions branch 3 times, most recently from 4beadc9 to 271c510 Compare September 12, 2018 17:42
Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

I look briefly into the executor. Please, consider comments below. The most important one is API proposal.

Why do you say here that the commit implements expression parser? It is not so, this commit implements only executor.

prev = prev or second_operand
end

-- Equal.
Copy link
Member

Choose a reason for hiding this comment

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

TBD: think how to handle nil / box.NULL with equal and other operation. Like SQL does, ternary logic?

@Totktonada
Copy link
Member

@Hollow111 BTW, how to break a line within an expression?

@Hollow111
Copy link
Contributor Author

@Hollow111 BTW, how to break a line within an expression?

If i get you right: just use multiline lua-string and break lines as usual (it behaves the same as space).

e.g.:

[[
    $var1 &&
    path.to.some.node
]]

@Totktonada
Copy link
Member

@Hollow111 Thanks! It seems that our graphql parser is not ready for this. I have filed the issue about is: #223. It should not be part of this PR, though.

@Hollow111 Hollow111 force-pushed the N_Tatunov/gh-13-c-style-expressions branch from 271c510 to 0895398 Compare September 14, 2018 00:45
Hollow111 and others added 5 commits September 14, 2018 07:55
Implement c-style filter expressions for graphql objects. Introduced
executor into expressions module. Moved regexp implementation into
utils.lua.

Needed for tarantool#13
@Totktonada Totktonada force-pushed the N_Tatunov/gh-13-c-style-expressions branch from 0895398 to ecdb29d Compare September 14, 2018 05:15
@Totktonada Totktonada added the enhancement New feature or request label Sep 14, 2018
@Totktonada Totktonada merged commit cf453c0 into tarantool:master Sep 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants