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

Call conf.clamp for puma options to convert the default :environment Proc into a String #2413

Merged
merged 2 commits into from
Oct 31, 2020

Conversation

ndbroadbent
Copy link
Contributor

@ndbroadbent ndbroadbent commented Oct 27, 2020

Hello,

I just saw a deprecation warning when running Puma 5.0.3 in Ruby 2.7: warning: deprecated Object#=~ is called on Proc. I initially raised the issue on the puma repo: puma/puma#2455
But it turns out that this was a bug in Capybara: puma/puma#2455 (comment)

Capybara was not calling #finalize_values to convert default Proc into a string. I've also tried to add a test to confirm that this is fixed, but not sure if it's the best way to test this.

@twalpole
Copy link
Member

I don't know that I agree it should be the calling codes responsibility to call finalize_values. I need to find time to go read the puma code and see whether we're using private apis or public.

@@ -28,6 +28,7 @@
options = default_options.merge(options)

conf = Rack::Handler::Puma.config(app, options)
conf.options.finalize_values
Copy link

Choose a reason for hiding this comment

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

@ndbroadbent ndbroadbent changed the title Call conf.options.finalize_values for puma options to convert the default :environment Proc into a String Call conf.clamp for puma options to convert the default :environment Proc into a String Oct 30, 2020
@ndbroadbent
Copy link
Contributor Author

Thanks @dentarg, you're right! This looks like the public API that we are meant to call. I have updated the PR

@ndbroadbent ndbroadbent force-pushed the finalize_puma_options branch 2 times, most recently from 49b0737 to 59de6e0 Compare October 30, 2020 08:52
@ndbroadbent
Copy link
Contributor Author

ndbroadbent commented Oct 30, 2020

Note: I opened a different PR to fix some RuboCop warnings on the master branch: #2417

I've also included that commit in here just to check that the build is also passing for this branch

@twalpole
Copy link
Member

Thanks

@twalpole twalpole merged commit 48bb29a into teamcapybara:master Oct 31, 2020
etiennebarrie added a commit to Shopify/maintenance_tasks that referenced this pull request Nov 7, 2020
lawrencewong pushed a commit to lawrencewong/maintenance_tasks that referenced this pull request Apr 29, 2023
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.

None yet

3 participants