-
-
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
Attemp to fix #866 and #867 #868
Conversation
@dblock Sorry, this is a incorrect fix. I'll find another way to fix this and rebase the commit. |
@@ -99,7 +99,6 @@ def reset_routes! | |||
def mount_in(route_set) | |||
if endpoints | |||
endpoints.each do |e| | |||
e.inheritable_setting.inherit_from inheritable_setting |
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.
@dblock Delete this line fix the issue, this line was introduced in this commit f8596829.
@dspaeth-faber Is this line necessary?
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 intention is to inherit everything when an API is mounted, but it looks like it has the exact opposite effect?
If we mount a Grape::API app which using version and namespaces to another Grape::API app, the version setting will lost.
We do need to figure out what is happening right now by inheriting everything from parent's inheritable settings and why it is causing the issue at hand. I am going to say that if all specs + new ones pass, we can take the fix. Can you please update CHANGELOG for this? |
@dblock updated. |
Merged via 0c0e85f. |
If we mount a Grape::API app which using version and namespaces to another
Grape::API app, the version setting will lost.