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

Ensuring we always have a known boolean operator #143

Merged
merged 12 commits into from
Aug 6, 2024
Merged

Conversation

valencik
Copy link
Collaborator

@valencik valencik commented May 29, 2024

This PR tackles a handful of issues:
(but hopefully the commits are still pretty isolated and clean 😬 )

  • Support configuring default boolean (OR / AND)
    • this is accomplished by making QueryParser a class with a defaultBooleanOR param
  • Changing Group to take only a single Query instead of a list
    • This addresses Problem 2 from Rethink Query hierarchy #142
    • the idea being that we should have a group of queries without knowing the boolean operator that connects them
    • Note: A MinimumMatch query is an exception to this, it contains a list of queries that truly are not linked by either an AND or OR
  • Removes MultiQuery
    • This addresses Problem 3 from Rethink Query hierarchy #142
    • Also part of the effort to not have lists of queries with unknown boolean operators

There remain a handful of TODO comments in the tests.
This are mostly around needlessly nested queries. But I think resolving these might make this PR even larger...

@valencik valencik self-assigned this May 29, 2024
@valencik valencik changed the title WIP Add support for configurable default boolean Ensuring we always have a known boolean operator Jul 13, 2024
@valencik valencik marked this pull request as ready for review July 18, 2024 16:00

class DefaultBooleanAndSuite extends munit.FunSuite {

val parser = new QueryParser(defaultBooleanOR = false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do find this defaultBooleanOR = false a bit awkward to work with implying that we have a parser with defaultBooleanAnd-like behaviour

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a convenience method to the companion object so this now looks like:

val parser = QueryParser.withDefaultOperatorAND

Which is a bit more explicit.

Copy link
Collaborator

@astronomerdamo astronomerdamo left a comment

Choose a reason for hiding this comment

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

Nice changes, thank you! Left an optional comment/question.

The default `QueryParser` automatically inserts an `OR` operation inbetween consecutive terms.

```scala mdoc
QueryParser.withDefaultOperatorOR.parse("cats dogs")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps worth noting that QueryParser.parse("cats OR dogs") also is OR by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe..... I don't know yet if we should keep the QueryParser.parse convenience forever...
So maybe, for now we encourage folks to be explicit with QueryParser.withDefaultOperatorOR.parse or QueryParser.default.parse and then eventually require it?

For now I think I prefer recommending the explicit thing.

@valencik valencik merged commit dc56dc8 into main Aug 6, 2024
25 checks passed
@valencik valencik deleted the default-bool branch August 6, 2024 22:58
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