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

CESQL #680

Merged
merged 9 commits into from
Apr 26, 2021
Merged

CESQL #680

merged 9 commits into from
Apr 26, 2021

Conversation

slinkydeveloper
Copy link
Member

@slinkydeveloper slinkydeveloper commented Apr 15, 2021

This PR provides a new package that implements CESQL spec.

The package passes all the tests of the TCK. This implementation is not super efficient, but I think it's good as a first step.

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

@slinkydeveloper slinkydeveloper added the enhancement New feature or request label Apr 15, 2021
@slinkydeveloper slinkydeveloper changed the title [WIP] CESQL CESQL Apr 19, 2021
@n3wscott
Copy link
Member

What is the code coverage look like on the new code?

@slinkydeveloper
Copy link
Member Author

What is the code coverage look like on the new code?

TCK covers around 80%, most non covered paths are the error paths (which just shortcircuit the computation)

@n3wscott
Copy link
Member

I wonder if nats changed something... or it needs more time... that is not related to your PR...

@n3wscott
Copy link
Member

I am assuming a readme will come in a followup, one nit is if you want that module to be released next push, please add the package path to the correct places in the ./hack/tag-release.sh script, but you can do this in a followup as well.

LGTM

@slinkydeveloper
Copy link
Member Author

I wonder if nats changed something... or it needs more time... that is not related to your PR...

There is an open issue #630

I am assuming a readme will come in a followup, one nit is if you want that module to be released next push, please add the package path to the correct places in the ./hack/tag-release.sh script, but you can do this in a followup as well.

Both fixed, can you check it? This should be ready for the first release, i also made very explicit that the package APIs are subject to changes.

Copy link
Member

@n3wscott n3wscott left a comment

Choose a reason for hiding this comment

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

LGTM

@n3wscott
Copy link
Member

you can follow the manual release process to produce a v2.4.1 release of the sql module, cherry-pick this commit to the release-2.4 branch.

@n3wscott
Copy link
Member

I landed the headers change, you can run the tool on the sql dir like: ./hack/boilerplate/add-boilerplate.sh go sql

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Implemented literal and negate expression

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper
Copy link
Member Author

Coyright headers done.

you can follow the manual release process to produce a v2.4.1 release of the sql module, cherry-pick this commit to the release-2.4 branch.

No need to have a release now about it. We can wait 2.5, the spec is still in the works.

import cesqlparser "github.com/cloudevents/sdk-go/sql/v2/parser"

// Parse the expression
expression, err := cesqlparser.Parse("subject = 'Hello world'")
Copy link
Member

Choose a reason for hiding this comment

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

mind adding a bit more of use cases, also more complex ones?

Sure: can be done in a follow up PR

@n3wscott
Copy link
Member

I would favor landing what you have and continuing to make PRs on it. We can choose to release it or not when it is time for the v2.5.0

LGTM

@slinkydeveloper slinkydeveloper merged commit 5198fc7 into cloudevents:main Apr 26, 2021
@slinkydeveloper slinkydeveloper deleted the cesql branch April 26, 2021 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants