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.

Sign up for GitHub

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

Jump to bottom

Support ruby 2.6 Hash#merge with multiple arguments #1572

Merged
merged 3 commits into from
Jan 1, 2020
Merged

Support ruby 2.6 Hash#merge with multiple arguments #1572

merged 3 commits into from
Jan 1, 2020
+27 −9

Conversation

iguchi1124
Copy link
Contributor

close #1571

I tested locally:

➜  sinatra bundle exec ruby test/indifferent_hash_test.rb
Run options: --seed 9474

# Running:

...........................

Finished in 0.004303s, 6274.6921 runs/s, 23471.9963 assertions/s.

27 runs, 101 assertions, 0 failures, 0 errors, 0 skips

@iguchi1124
Copy link
Contributor Author

iguchi1124 commented Oct 16, 2019 •
edited

test failing from this changes rack/rack#1391 👀

I cannot find out why Rack::Session::Abstract::SessionHash instance passes into its #stringify_keys method.

Apart from that, we using rack with latest revision, but I think there are no need to use latest revision.
This revision declare intended to use rack2 at December 2015.

https://github.com/sinatra/sinatra/blob/c174325/Gemfile#L14

maybe we can specify rack 2.x release version and avoid test failures? 🤔 abc0883

@jkowens
Copy link
Member

jkowens commented Oct 18, 2019

@iguchi1124 thanks for spotting the rack issue. I reported the bug and it was fixed there.

@iguchi1124
Copy link
Contributor Author

I reported the bug and it was fixed there.

thanks!

salzig
salzig reviewed Oct 31, 2019
View reviewed changes
lib/sinatra/indifferent_hash.rb Outdated
def merge!(*other_hashes)
other_hashes.each do |other_hash|
if other_hash.is_a?(self.class)
super
Copy link

Choose a reason for hiding this comment

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

wouldn't this call super with other_hashes, and thus merge all the arguments multiple times? So maybe super(other_hash) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

6f46895

jkowens
jkowens reviewed Nov 2, 2019
View reviewed changes
test/indifferent_hash_test.rb Outdated
assert_equal 1, hash[?a]
assert_equal 2, hash[?b]
assert_equal 3, hash[?c]
assert_equal 3, hash[?c]
Copy link
Member

Choose a reason for hiding this comment

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

This line appears to be a duplicate.

@jkowens
Copy link
Member

jkowens commented Nov 3, 2019

LGTM 👍

@namusyaka what do you think?

@iguchi1124 iguchi1124 changed the title Support ruby 2.6 merge with multiple arguments Support ruby 2.6 Hash#merge with multiple arguments Nov 18, 2019
namusyaka
namusyaka approved these changes Dec 30, 2019
View reviewed changes
Copy link
Member

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

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

LGTM, lemme check this in my local.

@namusyaka namusyaka merged commit d7a5b9c into sinatra:master Jan 1, 2020
@iguchi1124 iguchi1124 deleted the support-ruby26-merge branch January 1, 2020 14:56
@iguchi1124
Copy link
Contributor Author

Hi, I checked cf1c6b1 code which is released as v2.0.8, but these changes seems disappear. Could you check push logs? 🙏

@namusyaka

@namusyaka
Copy link
Member

Unfortunately, I can't see the push log because of the permission issue.
However, I just found the force-push message in this PR.
According to the message, I force-pushed the wrong commit revision in the master branch.

Based on the above assumption, I'm checking if there is other commit that was missing, but it's not found except this.
So, I'm going to re-release v2.0.8.1 for making this commit included in the version.

I'm sorry for my mistake on the release operation.

namusyaka added a commit that referenced this pull request Jan 1, 2020
@namusyaka
Merge pull request #1572 from iguchi1124/support-ruby26-merge
8252993 
Support ruby 2.6 Hash#merge with multiple arguments
namusyaka added a commit that referenced this pull request Jan 1, 2020
@namusyaka
Merge pull request #1585 from sinatra/fix-missing-commit
623a80b 
Merge pull request #1572 from iguchi1124/support-ruby26-merge
@namusyaka
Copy link
Member

Released: https://rubygems.org/gems/sinatra/versions/2.0.8.1
I confirmed that the released gem contains this change correctly.

Again, I was making a completely crazy mistake. Thanks so much for letting me know. And I'm really sorry.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Mar 20, 2020
www/ruby-sinatra: update to 2.0.8.1
610f760 
Update ruby-sinatra to 2.0.8.1.


## 2.0.8.1 / 2020-01-02

* Allow multiple hashes to be passed in `merge` and `merge!` for `Sinatra::IndifferentHash` [#1572](sinatra/sinatra#1572) by Shota Iguchi

## 2.0.8 / 2020-01-01

* Lookup Tilt class for template engine without loading files [#1558](sinatra/sinatra#1558). Fixes [#1172](sinatra/sinatra#1172) by Jordan Owens

* Add request info in NotFound exception [#1566](sinatra/sinatra#1566) by Stefan Sundin

* Add `.yaml` support in `Sinatra::Contrib::ConfigFile` [#1564](sinatra/sinatra#1564). Fixes [#1563](sinatra/sinatra#1563) by Emerson Manabu Araki

* Remove only routing parameters from @params hash [#1569](sinatra/sinatra#1569). Fixes [#1567](sinatra/sinatra#1567) by Jordan Owens, Horacio

* Support `capture` and `content_for` with Hamlit [#1580](sinatra/sinatra#1580) by Takashi Kokubun

* Eliminate warnings of keyword parameter for Ruby 2.7.0 [#1581](sinatra/sinatra#1581) by Osamtimizer

## 2.0.7 / 2019-08-22

* Fix a regression [#1560](sinatra/sinatra#1560) by Kunpei Sakai

## 2.0.6 / 2019-08-21

* Fix an issue setting environment from command line option [#1547](sinatra/sinatra#1547), [#1554](sinatra/sinatra#1554) by Jordan Owens, Kunpei Sakai

* Support pandoc as a new markdown renderer [#1533](sinatra/sinatra#1533) by Vasiliy

* Remove outdated code for tilt 1.x [#1532](sinatra/sinatra#1532) by Vasiliy

* Remove an extra logic for `force_encoding` [#1527](sinatra/sinatra#1527) by Jordan Owens

* Avoid multiple errors even if `params` contains special values [#1526](sinatra/sinatra#1527) by Kunpei Sakai

* Support `bundler/inline` with `require 'sinatra'` integration [#1520](sinatra/sinatra#1520) by Kunpei Sakai

* Avoid `TypeError` when params contain a key without a value on Ruby < 2.4 [#1516](sinatra/sinatra#1516) by Samuel Giddins

* Improve development support and documentation and source code by  Olle Jonsson, Basavanagowda Kanur, Yuki MINAMIYA
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@salzig salzig salzig left review comments

@jkowens jkowens jkowens left review comments

@namusyaka namusyaka namusyaka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support multiple arguments in IndifferentHash#merge!
4 participants
@iguchi1124 @jkowens @namusyaka @salzig