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

simplify retry #9

Merged
merged 1 commit into from
Jun 5, 2023
Merged

simplify retry #9

merged 1 commit into from
Jun 5, 2023

Conversation

dpep
Copy link
Contributor

@dpep dpep commented Jun 5, 2023

slimmed down version of #7 with additional test to catch regression. as described here, Rails expects subclasses of a Singleton to be instantiatable

@dpep dpep mentioned this pull request Jun 5, 2023
@ioquatix ioquatix merged commit a67bf24 into ruby:master Jun 5, 2023
matzbot pushed a commit to ruby/ruby that referenced this pull request Jun 5, 2023
@ioquatix
Copy link
Member

ioquatix commented Jun 5, 2023

This looks like a good first step to me - do you think we can adopt more of your previously proposed improvements?

@dpep dpep deleted the simplify-retry branch June 5, 2023 04:44
@dpep
Copy link
Contributor Author

dpep commented Jun 5, 2023

Not sure there's much more to do unless we want to change functionality, which isn't compelling. But let me know if you got ideas and we can iterate. Thanks for your help with this @ioquatix!

As a bonus, it looks like we inspired Rails to clean up their usage of Singleton! 🙌🏻 🧹

@ioquatix
Copy link
Member

ioquatix commented Jun 6, 2023

What I would suggest, is reopening a draft PR with your proposed changes, and just let it sit for until Rails 7.x is EOL, then we can merge it.

I'm sure any tests that you wrote would also be welcome.

The fact that rails deliberately re-introduced .new in a singleton class is clearly a misuse of the singleton pattern and I liked your simplifications, so we shouldn't let that prevent future adoption.

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

Successfully merging this pull request may close these issues.

2 participants