-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 broken multiple mounts. #2053
Conversation
4502ef0
to
9b1512b
Compare
e350198
to
0089ddd
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.
Good, thank you. Some nitpicky comments from me please before we can merge?
end | ||
end | ||
|
||
it 'does not raise a FrozenError on first instance' do |
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.
I understand that the bug is a frozen error raised, but let's rewrite this as a positive test of what you expect the response to be. Otherwise in the future when the behavior changes but no exceptions are raised we won't catch this issue.
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.
Correct me if I'm getting you wrong, but there is a positive and a negative test?
it 'responds the correct body at the first instance'
it 'does not raise a FrozenError on first instance'
Could you explain this in more detail please? :)
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.
The first covers the second in terms of functionality.
The second standalone is a misleading test. It would make sense if the call in some cases was supposed to raise FrozenError and we explicitly wanted to ensure that in certain conditions that's not the case.
0089ddd
to
4bd6502
Compare
You still have to remove the manually added |
4bd6502
to
415630c
Compare
Signed-off-by: Hermann Mayer <hermann.mayer92@gmail.com>
415630c
to
c141628
Compare
I merged this, thank you. |
Thank you! |
- What is it good for
There is a nasty bug on the
Grape::API::Instance#add_head_not_allowed_methods_and_options_methods
method which causes aFrozenError
when an API class is mounted multiple times. I'm not quite sure why the mounts happens multiple times at my setup. My application includes:This is the actual stack trace of the issue:
- What I did
The offending line at the
Grape::API::Instance
just assigns a frozen array constant to the routing settings. When usingCONST.dup
we can simply unfreeze it for later changes.- A picture of a cute animal (not mandatory but encouraged)