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

Removes DEPRECATION WARNING: find_by_name #195

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

filipemendespi
Copy link
Contributor

@filipemendespi filipemendespi commented Jan 19, 2022

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:

  • Replace find_by_name with find_by_unofficial_names per recommendation from gem Countries version 5.x

  Replace find_by_name with find_by_unofficial_names per recommendation from gem Countries version 5.x
@scudco
Copy link
Collaborator

scudco commented Jan 19, 2022

Thanks for this! It looks good to me.

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

@scudco scudco merged commit d75a064 into countries:master Jan 20, 2022
@pmor
Copy link
Member

pmor commented Jan 20, 2022

👋 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 filipemendespi deleted the deprecated-message branch January 20, 2022 15:31
@filipemendespi
Copy link
Contributor Author

@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
Copy link
Collaborator

scudco commented Jan 20, 2022

@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
Copy link
Collaborator

scudco commented Jan 20, 2022

@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
Copy link
Contributor Author

@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
Copy link
Member

pmor commented Jan 20, 2022

Try this instead:

config.country_priority = %w[CA]

@filipemendespi
Copy link
Contributor Author

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
Copy link
Collaborator

scudco commented Jan 20, 2022

@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
Copy link
Collaborator

scudco commented Jan 20, 2022

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
Copy link
Collaborator

scudco commented Jan 20, 2022

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

@pmor
Copy link
Member

pmor commented Jan 20, 2022

@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
Copy link
Contributor Author

@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.

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