countries / country_select

Gemification of rails's country_select
https://country-select-demo.onrender.com
MIT License
704 stars 198 forks source link

Removes DEPRECATION WARNING: find_by_name #195

Closed filipemendespi closed 2 years ago

filipemendespi commented 2 years ago

Hi, I'd like to thank the gem contributors for keeping the gem up to date.

I'm making this small change because when running tests in my application, I'm getting several deprecation warnings.

This change is referenced in this section of the Countries gem: https://github.com/countries/countries/#upgrading-to-42-and-5x

message error:

installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/countries-4.2.1/lib/countries/country/class_methods.rb:101: warning: DEPRECATION WARNING: 'find_by_name' and 'find_*_by_name' methods are deprecated, please refer to the README file for more information on this change.

Task:

scudco commented 2 years ago

Thanks for this! It looks good to me.

I'll try to push a new gem within the next 24 hours.

pmor commented 2 years ago

👋 I'm a maintainer of the countries gem. First off, nice catch, I missed it on #190

That being said, #find_by_unofficial_names will not work as expected. The deprecated method actually searches over several attributes, but for this purpose, #find_by_iso_short_name is probably what's needed

filipemendespi commented 2 years ago

@scudco Did you see the comment above, do you want me to make this change, or do you do it before generating a new version of the gem?

scudco commented 2 years ago

@pmor I did look at this and explored the find methods in countries a bit. Though I think find_by_iso_short_name may be technically correct, I believe this branch of the helper is exercised via other gems like formtastic which still use unofficial country names rather than codes. I think the correct thing to do here is to deprecate that branch of code within the country_select helper and drop support for name-based lookups in country_select 7.0. This should give me or someone else enough time to draft a PR for formtastic and simple_form if necessary.

See https://github.com/formtastic/formtastic/blob/e34baba470d2fda75bf9748cff8898ee0ed29075/lib/formtastic/form_builder.rb#L33

For reference

ISO3166::Country.find_by_iso_short_name('United Kingdom') # => nil
ISO3166::Country.find_by_iso_short_name('United Kingdom of Great Britain and Northern Ireland') # => ["GB", ...]
ISO3166::Country.find_by_iso_long_name('The United Kingdom of Great Britain and Northern Ireland') # => ["GB", ...]
ISO3166::Country.find_by_unofficial_names('United Kingdom') #=> ["GB", ...]
scudco commented 2 years ago

@filipemendespi can you post the invocation of country_select that was triggering the deprecation warning? I'm curious if this was happening through formtastic or your own invocation.

filipemendespi commented 2 years ago

@scudco We use simple_form within our project and the only configuration we have for the country is this:

config.country_priority = %w[Canada]

When I uncomment the line above the warnings stops passing, do you have any temporary solution for this?

And in the forms we use it like this:

<%= simple_form_for User.new, url: root_url do |f| %>
  <%= f.input :country %>
<% end %>
pmor commented 2 years ago

Try this instead:

config.country_priority = %w[CA]
filipemendespi commented 2 years ago

Try this instead:

config.country_priority = %w[CA]

Yes that solves the problem! Sorry, but the change seemed clear and I didn't pay attention! Thanks!

scudco commented 2 years ago

@pmor just checking I read understand this correctly–find_by_unofficial_names is not deprecated and will continue to be functionally equivalent to find_by_name, correct?

scudco commented 2 years ago

For future reference, here's some documentation in simple_form that should be updated eventually: https://github.com/heartcombo/simple_form/blob/86429bceb950096df3c29616f31bd5a5ce706c06/README.md#priority

scudco commented 2 years ago

@filipemendespi do you still receive deprecation warnings after upgrading to country_select 6.1.1?

pmor commented 2 years ago

@scudco Almost. #find_by_names was deprecated to #find_by_unofficial_names and they do the same, while #find_by_name is something I'm still thinking about a replacement for, since it searches in iso_short_name, unofficial_names and translated_names all at once.

Probably I'll add something that does what #find_by_name does, but with a name that is less ambiguous.

filipemendespi commented 2 years ago

@filipemendespi você ainda recebe avisos de descontinuação após atualizar para country_select 6.1.1?

@scudco No, the test I did at @pmor suggestion was version 6.1.0 and it solved the problem.