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

search_cities inconsistency #38

Closed
pigden opened this issue Jun 23, 2023 · 6 comments
Closed

search_cities inconsistency #38

pigden opened this issue Jun 23, 2023 · 6 comments

Comments

@pigden
Copy link
Contributor

pigden commented Jun 23, 2023

The function handles 'name' and 'alternatenames' differently, should these be brought in line with each other?

  • For 'name' that is a string it will do a string in string search. So can be a partial match for example 'new york' will return 'new york city')
  • For 'alternatenames' that is a list it will do a string in list search. So needs to be an exact match.

I am happy to implement bringing these two inline, however this would be a breaking functionality change unless you wish for it to be feature flagged (e.g. exact_match=None, that will keep the current difference but when set to true/false keeps them consistent)

@yaph
Copy link
Owner

yaph commented Jun 23, 2023

Making the behavior consistent makes sense. Adding an 'exact_match' flag is a good idea. I think it should either be True or False. Personally, I'd make False the default, although it is a breaking change.

Keeping the current inconsistency as the default would add complexity that seems unnecessary to me. I'm fine with creating a new major release. What behavior do you think should be the default?

Thanks for bringing this up and offering your help Chris!

@pigden
Copy link
Contributor Author

pigden commented Jun 28, 2023

Agree on keep current behaviour will add a lot of complexity, so doing a breaking change makes the most sense.

For me it would make sense to make the default True as the current default argument is 'alternatenames' that is an exact match search. With that said as we are making a breaking change it doesn't matter which one we go for.

A new version and breaking change may also be an opportunity to make case sensitive default to be False also.

Let me know what you think on the default value for 'exact_match' and ill send over the pull request.

@yaph
Copy link
Owner

yaph commented Jun 29, 2023

I'd choose False as the default for both case_sensitive and exact_match. I feel like that's what more people would expect. I'm wondering whether the term "exact_match" implies a case sensitive search, i.e. if I set "exact_match" to True and "London" as the search term, "london" would match. Could be confusing to some people, but I have no better term for exact_match.

@pigden
Copy link
Contributor Author

pigden commented Jun 29, 2023

Agreed, maybe we could go with one of the following 'contains_match=True', 'partial_match=True', 'contains_search=True'.

@yaph
Copy link
Owner

yaph commented Jun 29, 2023

I think I like 'contains_search=True' best.

@yaph
Copy link
Owner

yaph commented Jul 3, 2023

Just released version 2. Thank you for your contribution Chris!

@yaph yaph closed this as completed Jul 3, 2023
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

No branches or pull requests

2 participants