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

Add support to dependencies #129

Closed
wants to merge 1 commit into from
Closed

Conversation

Joxit
Copy link
Member

@Joxit Joxit commented Oct 6, 2021

I noticed that dependencies were often seen as countries, but it is not possible to use the boundary.country parameter to filter searches for documents that are located in dependencies.

Now the view boundary_country uses a multi_match query to match both country_a and dependency_a.

This code is fully backward compatible (needs API changes).

Copy link
Member

@missinglink missinglink left a comment

Choose a reason for hiding this comment

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

I like this change.

The documentation says that the boudary.country filter accepts an ISO code, so despite the word 'country' being a misnomer I think this filter should support both country and dependency.

As such, the filter is just the opposite of pelias/api#1541

Allowing backwards compatibility is a nice touch, but I'm not too fussed, it should really consider both fields by default.

@Joxit
Copy link
Member Author

Joxit commented Oct 6, 2021

Hum, ok so instead of retrieving fields using vars and configured in API package, I should add both fields here?

@missinglink
Copy link
Member

Good question @orangejulius how do you think we are best to handle this?

@orangejulius
Copy link
Member

Nice, this is great. Honestly, I think we can keep things simple and add the dependency field directly. Adding configuration is probably overkill.

@Joxit
Copy link
Member Author

Joxit commented Oct 6, 2021

Hum, it seems like search and autocomplete are using a different fields.

Search uses parent.country_a
https://github.com/pelias/api/blob/cba5c681a35e2cabba3b503ed7907eea45d97746/query/search_defaults.js#L60-L65

While autocomplete uses parent.country_a.ngram
https://github.com/pelias/api/blob/cba5c681a35e2cabba3b503ed7907eea45d97746/query/autocomplete_defaults.js#L70-L75

I don't know if a use parent.country_a instead of parent.country_a.ngram for autocomplet, this will break something...
On the other hand, using ngram for 3 letters would be a bit too much ? 🤔

@missinglink
Copy link
Member

missinglink commented Oct 7, 2021

hmm, so in general the autocomplete queries should use the ngram field, but in the case of the boundary.country filter it should only accept complete ISO 3166-2 or ISO 3166-3 codes, this behaviour should be the same between search and autocomplete.

Screenshot 2021-10-07 at 21 14 38

@Joxit Joxit force-pushed the joxit/feat/boundary.country branch from 14d2e20 to db61e51 Compare October 7, 2021 20:55
@Joxit
Copy link
Member Author

Joxit commented Oct 7, 2021

Excellent! I did the update 😉
The API PR is no longer needed \o/

@Joxit
Copy link
Member Author

Joxit commented May 9, 2022

Hi there, I will close this PR given that pelias/api#1622 has already been merged and does the job now.

@Joxit Joxit closed this May 9, 2022
@Joxit Joxit deleted the joxit/feat/boundary.country branch May 9, 2022 08:21
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