Further simplify singleton_class checking in class_attribute #4220

Merged
merged 1 commit into from Dec 28, 2011

Conversation

Projects
None yet
4 participants
Contributor

bensie commented Dec 28, 2011

We were having issues with the !name || '' == name simplification that @tenderlove committed for checking if we're in a singleton class. Though I was unable to come up with a failing test case, I was able to fix the issue and clean up the rails code a bit with this change.

If it's ok, I'd like to get this ported to 3-2-stable as well. Thanks!

@tenderlove tenderlove added a commit that referenced this pull request Dec 28, 2011

@tenderlove tenderlove Merge pull request #4220 from bensie/singleton-class-master
Further simplify singleton_class checking in class_attribute
90df0d4

@tenderlove tenderlove merged commit 90df0d4 into rails:master Dec 28, 2011

@jonleighton jonleighton added a commit that referenced this pull request Dec 28, 2011

@jonleighton jonleighton Revert "Merge pull request #4220 from bensie/singleton-class-master"
This reverts commit 90df0d4, reversing
changes made to 5e6fc81.

Reason: build breakage
9bae926
Contributor

tonycoco commented Dec 28, 2011

Can we reopen this pull request? I've seen this break in projects as well.

Contributor

bensie commented Dec 28, 2011

Yeah I'm working on another pull request. I broke the build :(

Contributor

tonycoco commented Dec 28, 2011

Shame on you. haha

Contributor

bensie commented Dec 28, 2011

@tenderlove Any chance we can just revert to:

  def singleton_class?
    # in case somebody is crazy enough to overwrite allocate
    allocate = Class.instance_method(:allocate)
    # object.class always points to a real (non-singleton) class
    allocate.bind(self).call.class != self
  rescue TypeError
    # MRI/YARV/JRuby all disallow creating new instances of a singleton class
    true
  end

This worked fine.

@tonycoco Can you produce a failing test that shows the breakage you're seeing? I've been unable to reproduce in the AS tests so far.

Contributor

tonycoco commented Dec 29, 2011

I'll give it a whirl later tonight.

Contributor

tonycoco commented Dec 30, 2011

Couldn't produce a test to capture the error yet. Even with a class_eval in the test it passed just fine. I'll keep looking.

Contributor

bensie commented Dec 31, 2011

@tonycoco Is the code where you're seeing failures public? Would be nice to see something other than CarrierWave that have this issue to help come up with a failing test.

Contributor

tonycoco commented Jan 3, 2012

It's not. I'm still trying to reproduce it.

Contributor

lest commented Jan 3, 2012

I think that we could check whether class is a singleton as following:

irb(main):008:0> klass = (class << Object.new; self end)
=> #<Class:#<Object:0x0000010084da68>>
irb(main):009:0> klass.ancestors.first == klass
=> false
irb(main):010:0> klass = Class.new
=> #<Class:0x000001010a5ff8>
irb(main):011:0> klass.ancestors.first == klass
=> true
Contributor

bensie commented Jan 3, 2012

I was just trying something along those lines. Running the Rails test suite with it now...

Contributor

bensie commented Jan 3, 2012

@lest Still failing singleton tests with that

Can we pleeeease just revert cdc4274? /cc @tenderlove @spastorino

Contributor

lest commented Jan 3, 2012

@bensie I've run carrierwave test suite with #4283 and only 1 test fails (there are 48 before)

Contributor

bensie commented Jan 3, 2012

CarrierWave passes, but Rails doesn't. Did you run the activesupport and actionpack tests?

Contributor

lest commented Jan 3, 2012

@bensie Sure I did. Looks weird.

Contributor

bensie commented Jan 3, 2012

I take it all back! #4283 works great. Can you merge and port to 3-2-stable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment