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

EZP-26123: Map facets of all types in ResultExtractor #52

Closed
wants to merge 1 commit into from

Conversation

wodor
Copy link

@wodor wodor commented Jun 29, 2016

This PR resolves https://jira.ez.no/browse/EZP-26123

This change allows mapping any type of facets as they come in facet_counts: "facet_queries", "facet_fields", "facet_dates", "facet_ranges" "facet_intervals"
This would be implementor's responsibility to map it properly, as the data structure is different.

For now, this is tested with visitors processing facet_fields and facet_intervals, if it gets positive feedback I can test if it's fine for other cases.

This change allows mapping any type of facets as they come in facet_counts:  "facet_queries", "facet_fields", "facet_dates", "facet_ranges"  "facet_intervals"
This would be implementor's responsibility to map it properly, as the data structure is different.
@wodor wodor changed the title Extract facets of all types Map facets of all types in ResultExtractor Jun 29, 2016
@pspanja
Copy link
Contributor

pspanja commented Jun 29, 2016

Hi @wodor, this looks good to me, do proceed :)

How do you plan to test it, by implementing missing facet types?

@wodor
Copy link
Author

wodor commented Jun 30, 2016

So, I have custom IntervalFacetBuilderVisitor like this https://gist.github.com/wodor/29576638a260187c678f18f5da2e11a3, but I'm not sure if it is in line with plans of development here.
What do you think ? Should something like this be added to the bundle ?

Beside this, I can write a unit test for the altered class.

@pspanja
Copy link
Contributor

pspanja commented Jun 30, 2016

It would first need to be added to the https://github.com/ezsystems/ezpublish-kernel, to become part of search API. But we are quite conservative with adding new API, since all of it has to be maintained, so I don't think it will happen without going through due process.

But we can definitely accept this change, there is no reason implementation should block you from implementing custom facets. Unit tests would be fine of course, though I think I would also be OK with merging this as is.

@pspanja
Copy link
Contributor

pspanja commented Jun 30, 2016

BTW, maybe you can put your custom facets in a public repo, I'm sure people would find it useful :)

@wodor
Copy link
Author

wodor commented Jul 1, 2016

OK, I'll try to add example implementation in a separate repo as a guideline.
Regarding unit tests, if you think it's OK to merge as is it's maybe better, I will be away for next two weekends.

@pspanja
Copy link
Contributor

pspanja commented Jul 5, 2016

Sorry for the delay @wodor.

+1, ping @alongosz @andrerom

@andrerom
Copy link
Contributor

Missing issue, unit test and pointing this to 1.0 branch. But we'll see if we can handle that in a few weeks if you're not available for it, if so we'll also look into adding integration tests.

@pspanja
Copy link
Contributor

pspanja commented Jul 19, 2016

Re integration tests, we do run them, only ones with missing implementation are skipped (TermFacetBuilder and FieldFacetBuilder). So this change is tested. This is the data provider: https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/API/Repository/Tests/SearchServiceTest.php#L2474

@andrerom
Copy link
Contributor

andrerom commented Jul 26, 2016

ok, @pspanja could you create issue for this so we can add the last remaining builders and also make sure this targets 1.0 branch?

@pspanja pspanja changed the title Map facets of all types in ResultExtractor EZP-26123: Map facets of all types in ResultExtractor Jul 26, 2016
@pspanja
Copy link
Contributor

pspanja commented Jul 26, 2016

Closing in favor of #56, opened against 1.0 branch
@wodor your commit is preserved of course

@pspanja pspanja closed this Jul 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants