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

fix for deprecation warnings in ruby 2.7.x #241

Closed
wants to merge 1 commit into from
Closed

fix for deprecation warnings in ruby 2.7.x #241

wants to merge 1 commit into from

Conversation

majksner
Copy link

@majksner majksner commented May 8, 2020

This pull request fixes the warnings for Ruby 2.7.x.

@codecov
Copy link

codecov bot commented May 8, 2020

Codecov Report

Merging #241 into dev will decrease coverage by 1.12%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##               dev     #241      +/-   ##
===========================================
- Coverage   100.00%   98.87%   -1.13%     
===========================================
  Files           26       17       -9     
  Lines          530      354     -176     
===========================================
- Hits           530      350     -180     
- Misses           0        4       +4     
Impacted Files Coverage Δ
lib/pagy/extras/shared.rb 77.77% <50.00%> (-22.23%) ⬇️
lib/pagy/extras/elasticsearch_rails.rb
lib/pagy/extras/support.rb
lib/pagy/extras/items.rb
lib/pagy/extras/searchkick.rb
lib/pagy/extras/pagy_search.rb
lib/pagy/extras/overflow.rb
lib/pagy/extras/trim.rb
lib/pagy/extras/headers.rb

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a09bb9...b852350. Read the comment docs.

@ddnexus
Copy link
Owner

ddnexus commented May 8, 2020

Doesn't that fail with 1.9 syntax?

@majksner
Copy link
Author

majksner commented May 8, 2020

Isn't the ruby 1.9 end of life?

@ddnexus
Copy link
Owner

ddnexus commented May 8, 2020

Regardless the end of life of 1.9, I wouldn't like to break systems that still use it, especially because pagy is backward compatible till 1.9 and I personally use it in legacy systems.

Besides, I just noticed that your change does actually change the semantic of the method, and does not work as it is supposed to work. def meth(*args) ... catches all the passed arguments in array, while def meth(**args) defines something completely different.

Indeed i18n tests fail with your change.

@ddnexus
Copy link
Owner

ddnexus commented May 8, 2020

And... I don't see any deprecation warning running the tests in 2.7.1 (https://travis-ci.org/github/ddnexus/pagy/jobs/678434972).

Where is your warning coming from?

@majksner
Copy link
Author

majksner commented May 8, 2020

/Users/nikola/.rvm/gems/ruby-2.7.1/gems/pagy-3.8.0/lib/pagy/extras/i18n.rb:14: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/nikola/.rvm/gems/ruby-2.7.1/gems/i18n-1.8.2/lib/i18n.rb:195: warning: The called method `t' is defined here

@majksner
Copy link
Author

majksner commented May 8, 2020

Seems it comes from https://github.com/ruby-i18n/i18n/

I'm sorry.

@ddnexus
Copy link
Owner

ddnexus commented May 8, 2020

It actually looks like it may come from how pagy calls the t method. The tests don't raise any warning, but their call might be different than in the actual call made in a page. I will check that. Thanks.

@ddnexus
Copy link
Owner

ddnexus commented May 8, 2020

It could be due to the i18n 1.8.2 version. Travis runs with 1.6.0. Please could you temporarily downgrade it to 1.6.0 and check if the warning is still there?

@majksner
Copy link
Author

majksner commented May 8, 2020

The warning is not displayed with i18n 1.6.0.

Check this out: https://github.com/ruby-i18n/i18n/blob/master/lib/i18n.rb#L188

@ddnexus
Copy link
Owner

ddnexus commented May 8, 2020

Your link points to the 1.8.2 code.

The 1.6.0 code defines the same arguments for the method I really don't get how it could not raise the same warning if you still run it on 2.7+.

The only difference about the versions is a comment added to the method (see ruby-i18n/i18n#501), so how comes that the warning disappears?

ddnexus added a commit that referenced this pull request May 8, 2020
…ast argument as keyword parameters" deprecation warning (closes #241)
@ddnexus ddnexus closed this May 8, 2020
@majksner majksner deleted the fix_deprecation_warnings branch May 8, 2020 11:42
@ddnexus
Copy link
Owner

ddnexus commented May 8, 2020

it breaks old rubies though :/

@majksner
Copy link
Author

majksner commented May 8, 2020

Maybe something like this can be used?

if RUBY_VERSION >= "2.7"
  def pagy_t_with_i18n(key, **opts) ::I18n.t(key, **opts) end
else
  def pagy_t_with_i18n(*args) ::I18n.t(*args) end
end

ddnexus added a commit that referenced this pull request May 8, 2020
…ast argument as keyword parameters" deprecation warning (closes #241)
@ddnexus
Copy link
Owner

ddnexus commented May 8, 2020

Thank you for the effort, but that would not avoid the syntax error for ruby <2.0.
The above commit does it.

@ddnexus
Copy link
Owner

ddnexus commented May 8, 2020

BTW, RUBY_VERSION >= "2.7" does not do what you think. :)

@ddnexus
Copy link
Owner

ddnexus commented May 8, 2020

Indeed the commit fixed it!

Thank you for the PR and your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants