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

Do nothing when the same block is included again #34553

Merged
merged 1 commit into from Nov 29, 2018

Conversation

mjtko
Copy link
Contributor

@mjtko mjtko commented Nov 28, 2018

If the same block is included multiple times, we no longer raise an exception or overwrite the included block instance variable.

Fixes #14802.

@simi
Copy link
Contributor

simi commented Nov 28, 2018

  1. What's the reason for this?
  2. Does this work in case when source location is the same, code changed, and reloaded?

@mjtko
Copy link
Contributor Author

mjtko commented Nov 28, 2018

@simi: Please review the discussion on #14802 for context.

Regarding (2) - it depends what you mean by "work". I don't think it works -- I've reopened a discussion about that aspect of this change on #14802, can you add your view there please?

activesupport/test/concern_test.rb Outdated Show resolved Hide resolved
activesupport/test/fixtures/some_module.rb Outdated Show resolved Hide resolved
@rafaelfranca
Copy link
Member

Can you squash your commits?

@mjtko
Copy link
Contributor Author

mjtko commented Nov 28, 2018

@rafaelfranca: yup, was just going to wait until they were all done first, but can do that now, np.

@gmcgibbon
Copy link
Member

Is the first commit necessary?

@mjtko
Copy link
Contributor Author

mjtko commented Nov 29, 2018

No, I just wanted to ensure the original author of the patch, @razum2um, received sufficient credit.

@gmcgibbon
Copy link
Member

gmcgibbon commented Nov 29, 2018

We usually give him credit in the commit message for the rails contributors app to pickup on. You can do [name & other name] or [name + other_name] in a commit message to credit multiple people. Example

If the same block is included multiple times, we no longer raise an exception
or overwrite the included block instance variable.

Fixes rails#14802.

[Mark J. Titorenko + Vlad Bokov]
@mjtko
Copy link
Contributor Author

mjtko commented Nov 29, 2018

Ok, thanks for the example! Squashed and modified accordingly. 😄

@rafaelfranca rafaelfranca merged commit c8e4d5a into rails:master Nov 29, 2018
rafaelfranca added a commit that referenced this pull request Nov 29, 2018
Do nothing when the same block is included again
@kalashnikovisme
Copy link

could you please tell me, how should I learn what Rails release has this fix?

@razum2um
Copy link
Contributor

razum2um commented Sep 10, 2019

@mjtko appreciate the credit, thanks 👍
surprisingly I noticed the thread only after last comment notification, sorry that I had to leave earlier PR & discussion.
long live rails community ❤️

dpzaba added a commit to dpzaba/couchrest_model that referenced this pull request May 6, 2020
…cument migrations

For compatibility issues with Rails version >= 4.1 and < 5.2.3
we have to avoid to load more than once the Rails concerns
rails/rails#34553
dpzaba added a commit to dpzaba/couchrest_model that referenced this pull request May 6, 2020
…cument migrations

For compatibility issues with Rails version >= 4.1 and < 5.2.3
we have to avoid to load more than once the Rails concerns
rails/rails#34553
dpzaba added a commit to dpzaba/couchrest_model that referenced this pull request May 6, 2020
…cument migrations

For compatibility issues with Rails version >= 4.1 and < 5.2.3
we have to avoid to load more than once the Rails concerns
rails/rails#34553
dpzaba added a commit to dpzaba/couchrest_model that referenced this pull request May 6, 2020
…cument migrations

For compatibility issues with Rails version >= 4.1 and < 5.2.3
we have to avoid to load more than once the Rails concerns
rails/rails#34553
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

6 participants