yaph / geonamescache

geonamescache - a Python library for quick access to a subset of GeoNames data.
https://pypi.org/project/geonamescache/
MIT License
99 stars 38 forks source link

search_cities inconsistency #38

Closed pigden closed 1 year ago

pigden commented 1 year ago

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

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

yaph commented 1 year ago

I think I like 'contains_search=True' best.

yaph commented 1 year ago

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