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

Remove outdated comment of ActiveRecord's spawn method. [ci-skip] #45335

Merged
merged 1 commit into from
Jun 12, 2022

Conversation

simi
Copy link
Contributor

@simi simi commented Jun 12, 2022

Per my findings, this method is not overridden anymore.

@kamipo
Copy link
Member

kamipo commented Jun 12, 2022

Duplicated with #41168.

@kamipo kamipo closed this Jun 12, 2022
@simi
Copy link
Contributor Author

simi commented Jun 12, 2022

Ahh, thanks for info @kamipo and sorry for wasting your time :(

If I understand it well, it is actually delegated to scope and that calls ActiveRecord::SpawnMethods#spawn again. Per the explicit comment I would expect custom spawn method implemented in Associations::CollectionProxy. It seems I'm not the only one confused by this comment. Would it be welcomed to at least reword the comment to make it easier to follow? Any ideas?

@kamipo kamipo reopened this Jun 12, 2022
@kamipo kamipo changed the title Remove outdated comment of ActiveRecord's spawn method. Remove outdated comment of ActiveRecord's spawn method. [ci-skip] Jun 12, 2022
@kamipo
Copy link
Member

kamipo commented Jun 12, 2022

Originally this private method existed just to emulate the equivalent behavior of a relation.clone on a collection proxy for internal use, but now exists for more complex purposes. I prefer to just remove it rather than comment on the exact purpose of this private method.

@kamipo kamipo merged commit 21651b0 into rails:main Jun 12, 2022
@simi simi deleted the spawn-docs-fix branch June 12, 2022 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants