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

deep_merge! uses missing method #507

Closed
flash-gordon opened this issue Jan 9, 2020 · 5 comments · Fixed by #509
Closed

deep_merge! uses missing method #507

flash-gordon opened this issue Jan 9, 2020 · 5 comments · Fixed by #509

Comments

@flash-gordon
Copy link

What I tried to do

Nothing, literally.

What I expected to happen

I receive no CI report.

What actually happened

i18n 1.8.0 was released and a scheduled build in dry-schema failed: https://github.com/dry-rb/dry-schema/runs/380987242
Backtraces lead to i18n where it calls deep_merge which is not defined.

Versions of i18n, rails, and anything else you think is necessary

i18n 1.8.0


Here's a piece of code containing the bug:

# deep_merge! from activesupport 5
# Copyright (c) 2005-2019 David Heinemeier Hansson
def deep_merge!(other_hash, &block)
merge!(other_hash) do |key, this_val, other_val|
if this_val.is_a?(Hash) && other_val.is_a?(Hash)
this_val.deep_merge(other_val, &block)
elsif block_given?
block.call(key, this_val, other_val)
else
other_val
end
end
end
The refinement doesn't include deep_merge so this code fails at line 26 in an environment missing activesupport extensions.

I'm sorry I didn't provide a repro as I don't have enough time atm but I hope this is enough.

@radar
Copy link
Collaborator

radar commented Jan 9, 2020

Reverted with #508. Pushed out with i18n 1.8.1 just now.

@CrAsH1101: would you happen to know why this has happened?

@radar
Copy link
Collaborator

radar commented Jan 9, 2020

Oh, looking at the code it looks like you will need to copy over the deep_merge method too. Please submit a PR for that if you wish to resurrect your other PR.

@vipera
Copy link
Contributor

vipera commented Jan 9, 2020

@radar I'll open the PR as this is my fault.

@CrAsH1101
Copy link
Contributor

Test suit includes active_support so this "bug" is only possible in projects which do not require active_support. Maybe active_support should not be included in tests?

@flash-gordon
Copy link
Author

Thanks folks! ❤️

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 a pull request may close this issue.

4 participants