-
Notifications
You must be signed in to change notification settings - Fork 6
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
Support for regex queries #188
Conversation
@valencik I finished added the changes you mentioned along with merging the latest changes from |
@VigneshSK17 Sorry for the delay, I should have some time to review and comment on this, this evening. |
No worries. I had to push a few extra changes to satisfy the CI checks but hopefully this covers all the necessary changes! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really good.
I think just the tweak regarding isolating the regex creation and we should be good.
try { | ||
val regex = q.str.r | ||
val terms = index.termDict.termsForRegex(regex) | ||
Right( | ||
terms | ||
.flatMap(m => index.docsWithTerm(m)) | ||
.toSet | ||
) | ||
} catch { | ||
case _: PatternSyntaxException => Left(s"Invalid regex query $q") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works but it puts a lot more stuff into the try
block than just the regex creation.
Can we isolate the try
block to just the regex creation?
Maybe something like:
val regex: Either[String, Regex] =
try {
Right(q.str.r)
} catch {
case _: PatternSyntaxException => Left(s"Invalid regex query $q")
}
Admittedly, part of me is wondering if there should be a toRegex
method on Query.TermRegex
itself...
But we can save that for another issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I was a doubtful about the large try-catch block.
I pushed code with a slight modification to the try-catch where instead of setting the type for regex as Either[String, Regex] it just return the error message for the entire function as it seemed redundant to match for the error and just return it unmodified. Let me know if I should avoid doing that for code readability, it's a quick change on my end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this
val regex =
try
q.str.r
catch {
case _: PatternSyntaxException => return Left(s"Invalid regex query $q")
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @VigneshSK17 for your work on this!
I've created an issue in the upstream query parser, Lucille, for the toRegex
helper method.
cozydev-pink/lucille#133
@valencik Sorry for bumping on this thread, but I was looking at the Scaladoc Search in Protosearch project idea and had a few questions about the project idea.
These questions would help me greatly with creating my proposal for Google Summer of Code. Thank you! |
Hey @VigneshSK17 let's tackle these questions in #203 |
As mentioned in #35