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

[sinatra-contrib][namespace] Do not raise when key is an enumerable #1619

Merged
merged 1 commit into from
Feb 12, 2021

Conversation

BuonOmo
Copy link
Contributor

@BuonOmo BuonOmo commented Jun 3, 2020

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 and sinatra-cross_origin doesn't work for local cross origin. Reproduction be like:

namespace "/foo" do
  get  "/bar" do
    cross_origin allow_origin: "example.com"
    send_file "bar"
  end
end

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 :)

@BuonOmo BuonOmo force-pushed the fix-namespace-set branch from b3d427b to 92f0104 Compare June 4, 2020 08:51
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.

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
Copy link
Member

@namusyaka namusyaka Jul 4, 2020

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?.

raise ArgumentError unless option.respond_to?(:each)

Copy link
Member

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?

Copy link
Contributor Author

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] }

@BuonOmo BuonOmo force-pushed the fix-namespace-set branch from 92f0104 to f7cfd89 Compare July 20, 2020 15:10
@BuonOmo BuonOmo force-pushed the fix-namespace-set branch from f7cfd89 to 282fb59 Compare July 20, 2020 15:32
@BuonOmo
Copy link
Contributor Author

BuonOmo commented Jul 20, 2020

@namusyaka done!

@namusyaka namusyaka self-requested a review November 11, 2020 16:58
@BuonOmo
Copy link
Contributor Author

BuonOmo commented Feb 10, 2021

Hey @namusyaka what is the status on that PR ? I'm currently cleaning some of my old PRs 😄

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.

looks good, thanks!

@namusyaka namusyaka added this to the v2.1.1 milestone Feb 12, 2021
@namusyaka namusyaka merged commit e79e9e6 into sinatra:master Feb 12, 2021
@BuonOmo BuonOmo deleted the fix-namespace-set branch February 13, 2021 17:57
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.

3 participants