-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[sinatra-contrib][namespace] Do not raise when key is an enumerable #1619
Conversation
b3d427b
to
92f0104
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Ulysse,
Thanks for your contribution.
Mostly looks good to me. However, you need to do a few actions like following:
- Please add a test regarding this change.
- Resolve my review comment.
@@ -284,8 +284,8 @@ def respond_to(*args) | |||
end | |||
|
|||
def set(key, value = self, &block) | |||
return key.each { |k,v| set(k, v) } if key.is_a?(Enumerable) and block.nil? and value == self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered the case that a given value has #each
but is not kind of Enumerable
?
For example, Sinatra::Base.set
checks whether the value has #each
method by calling respond_to?
.
Line 1267 in 91477e6
raise ArgumentError unless option.respond_to?(:each) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a look at this @BuonOmo
What do you think about using each_pair instead of each?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @namusyaka
If it feels important, please feel free to change it. I wouldn't since one may want to write something like:
set { foo: 1, bar: 5 }.map { |k, v| [k + "s", v.to_s] }
92f0104
to
f7cfd89
Compare
f7cfd89
to
282fb59
Compare
@namusyaka done! |
Hey @namusyaka what is the status on that PR ? I'm currently cleaning some of my old PRs 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, thanks!
Hi, and thanks for the great projet.
I made this first contribution because it feels like the order is wrong here: If a hash or array is given, the
set(k,v)
line would never be taken into account...In fact I came accross this issue because using both
sinatra/namespace
andsinatra-cross_origin
doesn't work for local cross origin. Reproduction be like:However, I do think that by design you want to block any global keys to be set from a namespace and do not have enough understanding of how this may work. Hence I'm thinking about fixing this issue directly in the
sinatra-cross_origin
gem. Any input on this one would be much appreciated of course :)