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

Add option to explicitly set the base class #20

Closed
wants to merge 2 commits into from
Closed

Add option to explicitly set the base class #20

wants to merge 2 commits into from

Conversation

fcofdez
Copy link

@fcofdez fcofdez commented May 30, 2013

There are some problems creating a pull request from issue using hub. It returns 500 error, so I've decided to create a new pull request.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 7b8514f on fcofdez:base-object into 1d41c1e on avdi:master.

end

it 'can be cloned' do
expect(null_instance.clone).to be_nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test might give you a false positive. Correct me if i'm wrong, but even if clone and dup don't work, they would still return nil because that's what this gem does.
I think clone should be tested around the freezed state of the new object

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, instances of singleton can't be duped or cloned

Copy link
Author

Choose a reason for hiding this comment

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

I was trying to keep coherence with the original gem behaviour, because if we don't mock dup and clone methods it will return an TypeError when we call dup or clone methods. Maybe the correct way of doing this should be letting singleton class to raise TypeError on dup or clone calls.

Copy link
Owner

Choose a reason for hiding this comment

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

I think the specdoc might be a little misleading. It is important for null objects to appear to be copyable, since they may well be asked to stand in for an object which is copyable. However, as you note, Singleton disables copying. These tests exist to ensure that we override the default Singleton behavior, and deliver a null object which acts like it is copyable.

Although actually now that I read this again, I think the current behavior is wrong; it should really return self from dup and clone, not nil. That way code that expected to be able to dup/clone some "real" object will Just Work with a singleton null object.

Copy link
Author

Choose a reason for hiding this comment

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

I think you are right, in my opinion Coherent behaviour should return self from dup and clone.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 93dab1e on fcofdez:base-object into 1d41c1e on avdi:master.

@avdi
Copy link
Owner

avdi commented May 30, 2013

Thanks! I changed the base class setting to be an accessor and merged.

@avdi avdi closed this May 30, 2013
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.

4 participants