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

Query Namespace #2645

Merged
merged 8 commits into from
Mar 2, 2021
Merged

Query Namespace #2645

merged 8 commits into from
Mar 2, 2021

Conversation

askvortsov1
Copy link
Sponsor Member

@askvortsov1 askvortsov1 commented Mar 1, 2021

**Part of flarum/issue-archive#286 **

Changes proposed in this pull request:
Move search results, search criteria, and ApplySearchParametersTrait to Query namespace.

They still extend (empty) search equivalents as a BC layer for extensions that expect those types.

Reviewers should focus on:
A few changes I'm considering, but am unsure about:

  • Moving FilterState to query. It's shared, but it's also an implementation detail.
  • Adding a QueryableInterface with a query method, and having both searchers and filterers implement it instead of filter and search methods. Leaning against this though because it isn't really necessary, and I don't see the benefits.

An actual extender for search drivers will have to wait until next release (or later), although that shouldn't be too difficult. Not a priority right now though.

@SychO9
Copy link
Member

SychO9 commented Mar 1, 2021

A few changes I'm considering, but am unsure about:
Moving FilterState to query. It's shared, but it's also an implementation detail.

I agree, what about having it and SearchState extend an AbstractQueryState ? might be better organized, FilterState would end up an empty class, but I think that'd be fine.

Adding a QueryableInterface with a query method, and having both searchers and filterers implement it instead of filter and search methods. Leaning against this though because it isn't really necessary, and I don't see the benefits.

Yea, We'd be overdoing it imo.

@askvortsov1 askvortsov1 merged commit a952691 into master Mar 2, 2021
@askvortsov1 askvortsov1 deleted the as/query-namespace branch March 2, 2021 14:57
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.

3 participants