Skip to content

singleton method_added fixes#1373

Merged
elia merged 1 commit intoopal:masterfrom
wied03:method_added_singleton
Mar 2, 2016
Merged

singleton method_added fixes#1373
elia merged 1 commit intoopal:masterfrom
wied03:method_added_singleton

Conversation

@wied03
Copy link
Copy Markdown
Contributor

@wied03 wied03 commented Mar 1, 2016

Ruby specs - ruby/spec#200

Another RSpec related issue

}

if (obj.$method_added && !obj.$method_added.$$stub) {
if (obj.$method_added && !obj.$method_added.$$stub && (obj.$$singleton_of !== Opal.BasicObject)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In light of what pointed out by @eregon in ruby/spec#200 (comment) this probably should look like

if (obj.$method_added && !obj.$method_added.$$stub && !obj.$$singleton_of) {
  
}
else if (obj.$singleton_method_added && !obj.$singleton_method_added.$$stub && obj.$$singleton_of) {
  
}

and that would probably fix some more ruby specs too… :)

@wied03 wied03 force-pushed the method_added_singleton branch from 3b67693 to 673b9cc Compare March 1, 2016 16:11
@wied03 wied03 changed the title Defining methods on BasicObject singleton should not trigger method_added singleton method_added fixes Mar 1, 2016
@wied03
Copy link
Copy Markdown
Contributor Author

wied03 commented Mar 1, 2016

@elia - Implemented your feedback. I've always found it a little odd to grasp the singleton class stuff, but as the code in this PR stands now. Should we be calling $singleton_method_added inside the else on obj instead? Or alternatively, should the else look like this?

else if (singleton_of && singleton_of.$singleton_method_added && !singleton_of.$singleton_method_added.$$stub)

@elia
Copy link
Copy Markdown
Member

elia commented Mar 1, 2016

Honestly I'm not sure without delving into it again, in doubt skip the else :) 

@wied03
Copy link
Copy Markdown
Contributor Author

wied03 commented Mar 1, 2016

It passes the rubyspecs either way, which makes me uncomfortable. Maybe I can dig in and find out why.

@eregon
Copy link
Copy Markdown
Contributor

eregon commented Mar 1, 2016

@wied03 I think your condition above in the comment is good.
obj is a Module or a Class here, which might be confusing naming, and singleton_of is the singleton class' instance AFAIK. singleton_method_added is called on the singleton instance, not a module.
The tests pass because modules/classes respond to singleton_method_added since they are BasicObject.

Latest rubyspecs, filter out new ruby specs that we don't pass
@wied03 wied03 force-pushed the method_added_singleton branch from 673b9cc to e6fd395 Compare March 1, 2016 16:25
@wied03
Copy link
Copy Markdown
Contributor Author

wied03 commented Mar 1, 2016

OK. That makes sense. I pushed that change as well and squashed the commits.

elia added a commit that referenced this pull request Mar 2, 2016
@elia elia merged commit 4a6fb4e into opal:master Mar 2, 2016
@wied03 wied03 deleted the method_added_singleton branch March 2, 2016 15:33
wied03 added a commit to wied03/opal that referenced this pull request Mar 2, 2016
elia added a commit that referenced this pull request Mar 2, 2016
Documentation for #1373 [skip ci]
hmdne pushed a commit to hmdne/opal that referenced this pull request Jan 27, 2024
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