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

Actions should be optional even if the rule has a where clause #78

Closed
wants to merge 1 commit into from
Closed

Actions should be optional even if the rule has a where clause #78

wants to merge 1 commit into from

Conversation

alexkolson
Copy link
Contributor

This is my attempt at fixing #62. I'm by no means an expert on antlr but I want to see if my thought here works. The thought being that only the parser code referencing WHERE includes event_expression, all others include event_filter. It looks like, if I'm understanding correctly, that event_expression is requiring an action?

Anyway, if nothing else this is a starting point from which we can make a better solution.

Sidenote: How do we test this effectively? I'd like to avoid standing up a VM just for a local KRE environment (if possible). Making a KRE docker image sounds promising.

@windley
Copy link
Member

windley commented May 4, 2015

This won't work (in fact doesn't...I tested it).

The event_expression is what allows you to write url.match(re/foo/) after the where clause. Event filter won't allow general expressions there.

As you say, testing this and working on it without a test environment is difficult. The Docker KRE build isn't too far from being done. I was originally going to put Mongo inside it (that's how it's build now), but I think better to run the Mongo docker container separately using a standard Mongo docker image. That's what I've been playing with lately.

@windley
Copy link
Member

windley commented May 4, 2015

BTW, if you want a VM to test KRE changes, until the docker stuff is done, I can give you an image you can start from if you like.

@alexkolson
Copy link
Contributor Author

Yeah that would be cool.

@alexkolson
Copy link
Contributor Author

Closing this because it doesnt work.

@alexkolson alexkolson closed this May 10, 2015
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