Skip to content

simplify #7

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

Merged
merged 1 commit into from
Jun 3, 2023
Merged

simplify #7

merged 1 commit into from
Jun 3, 2023

Conversation

dpep
Copy link
Contributor

@dpep dpep commented Jun 2, 2023

I was trying to understand the Singleton code (neat!) and saw a few ways we could simplify. If we roll Singleton.__init__ into included, it means we don't have to expose that helper method to the public and creates a single entry point (include Singleton). We could also simplify the instance method to have only a single return statement, at the end. I also added some tests that helped me catch a regression during development.

(ported from ruby/ruby#7890 and thanks to @jeremyevans and @ioquatix for taking a first pass!)

Would this help?

@dpep dpep mentioned this pull request Jun 2, 2023
@ioquatix
Copy link
Member

ioquatix commented Jun 2, 2023

This looks good to me, can you considerate feedback of #clone - do we need to add a test to confirm it's working correctly?

@dpep
Copy link
Contributor Author

dpep commented Jun 2, 2023

@ioquatix if I understand @jeremyevans's feedback around #clone, then this new test catches the regression he pointed out and confirms this change is working correctly, ie. that cloning a singleton class creates a new singleton class with it's own unique instance. (note that I added back in @singleton__instance__ = nil to resolve this)

does that address your concern or is there another test case we should add?

@ioquatix ioquatix merged commit 545b6b6 into ruby:master Jun 3, 2023
matzbot pushed a commit to ruby/ruby that referenced this pull request Jun 3, 2023
(ruby/singleton#7)

Remove `__init__` and move logic to `included`.
@dpep dpep deleted the simplify branch June 3, 2023 15:41
hsbt added a commit that referenced this pull request Jun 5, 2023
@hsbt
Copy link
Member

hsbt commented Jun 5, 2023

I reverted this change because this break Rails CI.

see https://bugs.ruby-lang.org/issues/19711

@ioquatix
Copy link
Member

ioquatix commented Jun 5, 2023

@hsbt thanks.

@ioquatix
Copy link
Member

ioquatix commented Jun 5, 2023

@dpep your change was reverted because it broke Rails. That means the behaviour of your implementation changed, and unfortunately it was not covered by a test in this repository.

https://bugs.ruby-lang.org/issues/19711

It looks like Rails expects subclasses of a Singleton to be instantiatable.

If you have time, can you investigate?

@dpep dpep mentioned this pull request Jun 5, 2023
@dpep
Copy link
Contributor Author

dpep commented Jun 5, 2023

whoops, sorry about that! see #9 for a retry with improved test coverage to guard against this corner case

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