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

(2.22.0) Adds: missing query parameters #600

Merged
merged 5 commits into from
Jul 10, 2019
Merged

(2.22.0) Adds: missing query parameters #600

merged 5 commits into from
Jul 10, 2019

Conversation

Ant-hem
Copy link
Member

@Ant-hem Ant-hem commented Jul 10, 2019

Fix #599

Q A
Bug fix? yes
New feature? no
BC breaks? no
Related Issue Fix #599
Need Doc update no

@Ant-hem Ant-hem requested a review from aseure as a code owner July 10, 2019 08:29
@Ant-hem Ant-hem changed the base branch from master to 2.X July 10, 2019 08:29
@Ant-hem Ant-hem force-pushed the missing-params-v2 branch 2 times, most recently from cc48eee to 03e0ec1 Compare July 10, 2019 10:19
Copy link

@aseure aseure left a comment

Choose a reason for hiding this comment

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

Great changes, thank you Antoine!

Only comment but maybe too late: we could have split this in multiple commits (one of each type added/changed and one for the tests) and some for the tests. This way, once we generate the ChangeLog entry, we could directly let users see what has changed (especially as we are introducing breaking changes). WDYT?

.collect(Collectors.joining(","))
+ "]")
.collect(Collectors.joining(","))
+ "]";
Copy link

Choose a reason for hiding this comment

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

I don't know if this style is enforced by coveo but it is particularly difficult to read. May we try to change that?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's coveo styling convention and I couldn't find a way to change it.

Breaking change:
- numericFilters is now List<List<String>> instead of List<String>
- tagFilters is now List<List<String>> instead of List<String>
- analyticsTags is now List<String> instead of String.

This is also adding support for nested list when serializing the query
as urlParameters.
@aseure aseure merged commit d661784 into 2.X Jul 10, 2019
@aseure aseure deleted the missing-params-v2 branch July 10, 2019 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants