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 keyword arguments for future compatibility #5175

Closed
wants to merge 1 commit into from

Conversation

fursich
Copy link

@fursich fursich commented Dec 29, 2019

Hi, thanks for your great effort to maintain this gem!

summary

Just found out a warning about keyword arguments that will potentially break with future ruby, when trying to test devise against Ruby2.7.
(thankfully the prior PR #5174 covered most of them - examining the test logs that I ran with my forked repo, this seems the only one left out)

why it's needed to change

Devise::IntegrationTest and Devise::ControllerTestCase offers sets of methods that delegate its execution to ActionController::TestCase.

As the delegated methods (and its delegated one, #process) require keyword arguments, we need to pass our hash objects as keyword arguments.

compatibility

Lots of corner cases have been discussed around this issue, luckily this patch is perfectly compatible with the current implementation, except for the deprecation warnings emitted by the existing codes (when being run on Ruby2.7).

Specifically:

  • When keyword arguments or hash object are provided

    • devise helper methods delegate its argument and keyword arguments to ActionController::TestCase#get, #post, etc.
    • for current implementation, warnings are emitted with Ruby2.7
  • When no keyword arguments are given

    1. with < Ruby2.7
      • both implementation (current vs new) passes its argument with empty hash.
    2. with Ruby2.7.0
      • both implementation (current vs new) passes its argument only. the behavior differs from that of older rubies, but both new and current implementation shows this behavior change. Delegated methods can run without problems in all versions of Ruby.

the latter case is the only corner case. With Ruby < 2.7, we pass over empty hash to ActionController::TestCase, but it does no harm as it is interpreted as empty keyword args with < Ruby 2.7.

backup

To demonstrate how they compare (and for my own peace of mind), I created a few delegation test cases - emulating current implementation, and new implementation , as well as cases about how they compare against each other

These cases are tested against Ruby2.1.10 upto 2.7.0 with similar environment with devise. Please feel free to check in case you are interested ;)

Hope that helps!

@sourcelevel-bot
Copy link

Hello, @fursich! This is your first Pull Request that will be reviewed by SourceLevel, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

@@ -11,7 +11,7 @@ class IntegrationTest < ActionDispatch::IntegrationTest
if options.empty?
super url
else
super url, options
super url, **options
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only Rails 5+ are required to cope with Ruby2.7 and above (no need to change below part)

@@ -36,7 +36,7 @@ class ControllerTestCase < ActionController::TestCase
if options.empty?
super action
else
super action, options
super action, **options
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, only Rails5.x and above are required to adapt to Ruby2.7+

@carlosantoniodasilva
Copy link
Member

@fursich my apologies, I didn't see your PR here and I fixed this directly on master: 34d9053, by removing those helpers entirely. They don't seem to be required on Rails 5+, as it's appears to handle passing the args directly just fine.

Thanks for your contribution.

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

Successfully merging this pull request may close these issues.

2 participants