-
-
Notifications
You must be signed in to change notification settings - Fork 390
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
Add spec for defined?
on constant with const_missing
#1215
Add spec for defined?
on constant with const_missing
#1215
Conversation
language/defined_spec.rb
Outdated
@@ -900,6 +900,14 @@ | |||
defined?(DefinedSpecs::Undefined).should be_nil | |||
end | |||
|
|||
it "returns nil when the constant is not defined and the parent implements const_missing" 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.
The term parent
is not accurate. It should be about outer scope/module/etc of a constant.
minor: it's common to prefix a method name with either .
or #
in a spec title so const_missing
becomes . const_missing
.
end | ||
|
||
defined?(DefinedSpecs::Undefined).should be_nil | ||
end |
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.
Dynamic defining a new method in a class/module shared by multiple test cases may make them surprisingly coupled, what makes sense to avoid.
So I would add a new fixture module/class specifically for this test case, e.g. DefinedSpecs::ModuleWithConsMissing
instead.
@andrykonchin Thanks for the review! Actually I've looked at the spec again and there are already tests to check that The current tests use a |
Yeah, I would say it makes sense to add this test case. At least it would catch the issue in your case. There might be a tiny difference between semantic of the new test and the existing one. Actually the |
Make sure that `defined?` returns nil even if `const_missing` is implemented in the outer module.
4cbc05a
to
ece2bf4
Compare
Ok I've implemented the changes you requested, let me know if anything is missing! |
Thank you! Looks good. |
Make sure that
defined?
returns nil even ifconst_missing
is implemented in the parent.We noticed this while implementing
defined?
in Natalie (see natalie-lang/natalie#2289, natalie-lang/natalie#1734)