Support Ruby 3 keyword arguments #97
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since Ruby 2.7, `[**empty_hash]` returns `[]` instead of `[{}]`.
FYI: `[{ **f1, **f2 }]` also works.
The code depends upon `patterns_from(pattern, **empty_hash)` that passes an empty hash as the second argument. Since 2.7, it passes only one positional argument. I think that the intention here is to skip pattern generation when `options` are empty and when `pattern` is a Pattern object. This changeset makes it more explicit.
The code expects `foo("a" => 1, :b => 2)` to pass one positional
argument `{"a"=>1}` and one keyword argument `{:b=>2}`, but
unfortunately, this behavior of Hsah separation was a bug of Ruby 2.6 or
prior. Since Ruby 2.7, non-Symbol key is allowed as keyword arguments,
so both are passed as keyword arguments.
This changeset adds braces to some method calls to pass a Hash object.
Passing keyword arguments must be explicitly marked as `**` (or no braces). https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/
|
@mame don't you need to add the |
|
@dentarg Yes, I will do later. Thank you dor pointing that out. |
|
The keyword argument separation issues in mustermann that this pull request fixes are currently blocking the separation of positional and keyword arguments in Ruby 3 development (ruby/ruby#2794). If this pull request could be given priority, I would appreciate it. @mame, could you add a commit that updates the gemspec? @dentarg, is there anything else that needs to be done before this is merged? |
|
Sorry, I forgot it! Will do soon |
|
@jeremyevans Not sure I can answer that, I just happened to see this one thing, other than that, I'm not really familiar with mustermann. It's @namusyaka that's been merging the most recent PRs. Perhaps @jkowens (or @zzak) can also help here? |
|
This looks good to me. I'd be fine merging it in, but I don't think I have access to do any releases. So I guess that's not super helpful 😦 Hopefully @namusyaka will have a chance to chime in. |
|
Will do soon. Sorry for the late action. |
namusyaka
approved these changes
Dec 30, 2019
| its(:to_h) { should be == { Mustermann.new("/foo") => Mustermann::Expander.new("/bar") } } | ||
| example { mapper['/foo'].should be == '/bar' } | ||
| example { mapper['/fox'].should be == '/fox' } | ||
| end | ||
|
|
||
| context 'allows specifying type' do | ||
| subject(:mapper) { Mustermann::Mapper.new(additional_values: :raise, type: :rails, "/foo" => "/bar") } | ||
| subject(:mapper) { Mustermann::Mapper.new({ "/foo" => "/bar" }, additional_values: :raise, type: :rails) } |
There was a problem hiding this comment.
Do we need to update an example here?
|
@mame @jeremyevans Now the PR has been merged, and a new version including the changes has been released: https://rubygems.org/gems/mustermann/versions/1.1.0 |
ghost
pushed a commit
to rubygems/bundler
that referenced
this pull request
Dec 31, 2019
7526: Fix CI failure with mustermann 1.1.0 r=deivid-rodriguez a=kou ### What was the end-user problem that led to this PR? This PR doesn't fix any end-user problem. This PR fixes a developer problem. Our CI is failed with mustermann 1.1.0. ### What was your diagnosis of the problem? This is caused because mustermann 1.1.0 starts depending on ruby2_keyword gem but our test script doesn't add a load path for ruby2_keyword gem yet. See also: * The PR that adds ruby2_keyword gem dependency to mustermann: sinatra/mustermann#97 * Error message on CI: #7523 (comment) ### What is your fix for the problem, implemented in this PR? My fix adds a load path for ruby2_keywrod gem installed in `tmp/1/gems/`. ### Why did you choose this fix out of the possible options? I don't have another option. Co-authored-by: Sutou Kouhei <[email protected]>
deivid-rodriguez
pushed a commit
to rubygems/bundler
that referenced
this pull request
Jan 1, 2020
7526: Fix CI failure with mustermann 1.1.0 r=deivid-rodriguez a=kou ### What was the end-user problem that led to this PR? This PR doesn't fix any end-user problem. This PR fixes a developer problem. Our CI is failed with mustermann 1.1.0. ### What was your diagnosis of the problem? This is caused because mustermann 1.1.0 starts depending on ruby2_keyword gem but our test script doesn't add a load path for ruby2_keyword gem yet. See also: * The PR that adds ruby2_keyword gem dependency to mustermann: sinatra/mustermann#97 * Error message on CI: #7523 (comment) ### What is your fix for the problem, implemented in this PR? My fix adds a load path for ruby2_keywrod gem installed in `tmp/1/gems/`. ### Why did you choose this fix out of the possible options? I don't have another option. Co-authored-by: Sutou Kouhei <[email protected]> (cherry picked from commit 984bef1)
ghost
pushed a commit
to rubygems/rubygems
that referenced
this pull request
Mar 11, 2020
7526: Fix CI failure with mustermann 1.1.0 r=deivid-rodriguez a=kou ### What was the end-user problem that led to this PR? This PR doesn't fix any end-user problem. This PR fixes a developer problem. Our CI is failed with mustermann 1.1.0. ### What was your diagnosis of the problem? This is caused because mustermann 1.1.0 starts depending on ruby2_keyword gem but our test script doesn't add a load path for ruby2_keyword gem yet. See also: * The PR that adds ruby2_keyword gem dependency to mustermann: sinatra/mustermann#97 * Error message on CI: rubygems/bundler#7523 (comment) ### What is your fix for the problem, implemented in this PR? My fix adds a load path for ruby2_keywrod gem installed in `tmp/1/gems/`. ### Why did you choose this fix out of the possible options? I don't have another option. Co-authored-by: Sutou Kouhei <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR allows rspec to pass in Ruby 2.7.
FYI: https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/