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 additional Ruby 2.7 keyword warnings #1586

Merged
merged 2 commits into from Mar 13, 2020

Conversation

@stefansundin
Copy link
Contributor

@stefansundin stefansundin commented Jan 2, 2020

These were missed in #1581.

@namusyaka
Copy link
Contributor

@namusyaka namusyaka commented Jan 2, 2020

If you hope eliminating the warnings, why didn't you fix other points such as "send_file"?

@namusyaka
Copy link
Contributor

@namusyaka namusyaka commented Jan 2, 2020

Also, AFAIK, the "opt = {}" with keyword args will be kept as compatibility layer as per https://blog.saeloun.com/2019/10/07/ruby-2-7-keyword-arguments-redesign#compatibility-layer

So I guess we don't have to merge this for v2.7, right?

@stefansundin
Copy link
Contributor Author

@stefansundin stefansundin commented Jan 2, 2020

I got the warnings from sinatra/reloader when loading my app in dev mode. I was curious, so I tried to require all the sintra-contrib files, and the only other one that gave me a warning was sinatra/respond_with.

I don't know why the other methods with options = {} don't trigger the warning, but these three definitely do. As for why I didn't fix send_file, I don't think it requires fixing. Please double check this though.

And like that other PR that is already merged, this isn't strictly required, it just prints an annoying warning when the files are included with require.

@namusyaka
Copy link
Contributor

@namusyaka namusyaka commented Jan 7, 2020

@stefansundin Thanks.

@jkowens Could you check in term of other things.
Also I would like you to check the thing I pointed out (e.g. send_file).

@stefansundin
Copy link
Contributor Author

@stefansundin stefansundin commented Feb 19, 2020

@jkowens Could you take a look at this? I think we can merge it. Thanks!

@jkowens
Copy link
Member

@jkowens jkowens commented Mar 11, 2020

@stefansundin I found two more locations that are throwing warnings when I run the tests. Can you fix those as well? 🙏

handler.run(self, server_settings) do |server|

result = base.send(method, pattern, conditions, &block)

@stefansundin
Copy link
Contributor Author

@stefansundin stefansundin commented Mar 11, 2020

Hi @jkowens. I'm having issues installing libv8 on my machine, so I can't test it unfortunately. Could you tell me exactly what to change those two lines to?

I think you can also propose the exact changes in a review: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/reviewing-proposed-changes-in-a-pull-request

And since you are a member of the Sinatra org, I think you have write access already to this branch. Feel free to push to it yourself.

@jkowens jkowens force-pushed the stefansundin:fix-keyword-warnings branch from 5e5fc03 to 4ac42bb Mar 13, 2020
@jkowens
Copy link
Member

@jkowens jkowens commented Mar 13, 2020

Ok, I ended up only updating one other location. The warning at sinatra/base.rb#L1526 seems like it will need to be addressed by the rack handlers (Puma, etc).

@jkowens jkowens merged commit 1f29a6d into sinatra:master Mar 13, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dentarg dentarg mentioned this pull request May 6, 2020
@mauro-oto mauro-oto mentioned this pull request Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.