Closed jhorstmann closed 4 years ago
+1
Sorry for late response, looks like i have accidentally unwatched own repository, didn't even know it is possible. Nice to see someone is already trying to use it.
Currently you can workaround it with label labelname=whereClause?
and you will be able to get field QueryContext::labelname
with type Option<Rc<WhereClauseContextAll>>
Also it feels that when using labels, parse tree structure is more idiomatic for Rust and it is faster because there is no need to look through children. I am thinking about adding an option that will generate fields instead of getters even without a label.
Nevertheless I think you are right and it should be added, but on the other hand it will add syntax overhead(.as_ref().unwrap()
) in cases where rule is not optional. It would be good to wrap type in Option
only when rule is optional. Not sure if it is possible to get that information in the place it is generated from, though.
Thanks a lot. I updated to the latest version which made all return types into Option
and it works for my usecase. There is a bit of syntactic overhead since I try to avoid unwrap
and instead do something like
opt_ctx.ok_or_else(|| ParserErrors::ParseError("Could not parse ...".to_owned()))?
but that looks fine to me and I could probably simplify it a bit with some abstractions. If you want you can close this issue.
First of all, thanks for this implementation, it is already working quite well. I'm running into some issues however with optional rules and I saw that a similar change happened recently for optional tokens.
Consider the following grammar:
Currently there is no way to check whether the
whereClause
rule matched since the return type ofQueryContextAttrs::whereClause
isRc<WhereClauseContextAll>
and would panic if called without matches. I think this also needs to be wrapped into anOption
. That would also match the java implementation, which would returnnull
when calling that method.As a workaround I could probably inline the rule and check for presence of the
WHERE
token first, but that might not work in all cases.