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

Fixes issue by checking if there's a base is there at all #1946

Merged
merged 3 commits into from
Dec 17, 2019

Conversation

nicolashopin
Copy link
Contributor

Addresses issue raised here: #1945

The cause of the issue is the introduction of the LazyBlock where blocks would only be excecuted on properly mounted instances, the side effect is for people using just Instance they had no base at all.

CHANGELOG.md Outdated
@@ -13,7 +13,7 @@

#### Fixes

* Your contribution here.
* [#1931](https://github.com/ruby-grape/grape/pull/1946): Fixes issue when using namespaces in Grape::API::Instance mounted directly - [@myxoh](https://github.com/myxoh)
Copy link
Member

Choose a reason for hiding this comment

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

Needs a period. Quote Grape::API::Instance.

@@ -121,7 +121,7 @@ def evaluate_as_instance_with_configuration(block, lazy: false)
self.configuration = value_for_configuration
response
end
if base_instance? && lazy
if base && base_instance? && lazy
Copy link
Member

Choose a reason for hiding this comment

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

Should we be doing this check inside base_instance?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it makes sense, as base_instance? usually means: "This instance is not used anywhere and is just a template for other instances" while, if an API has no base, it will most likely be used directly

@dblock dblock merged commit c7f0ac2 into ruby-grape:master Dec 17, 2019
@dblock
Copy link
Member

dblock commented Dec 17, 2019

Thanks for a quick fix!

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