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

Monkeypatch of Virtus for empty arrays to be coerced to nil causes unintended consequences #136

Open
ghost opened this issue Oct 2, 2019 · 4 comments

Comments

@ghost
Copy link

ghost commented Oct 2, 2019

There was a similar issue with #113 which was fixed by patching the monkeypatch. But we are using a gem which expects empty arrays to stay as empty arrays.

The discussion here: 27599e7#r16125838 suggests that the required coercion is different from what Virtus does.

My suggestion is to create the NullifyEmpty class as suggested and move TrackerAPI specific behavior into it. Otherwise TrackerAPI is conflicting with other gems.

The behavior needed is a Custom Coersion which would be added to the Shared::Base

@ghost ghost changed the title Virtus monkeypatch of empty arrays to nil causes unintended consequences Monkeypatch of Virtus for empty arrays to be coerced to nil causes unintended consequences Oct 3, 2019
@ghost
Copy link
Author

ghost commented Oct 4, 2019

@forest The other option is to use Ruby refinements which would limit the scope of the monkeypatch. I will play with it and see if that can work.

@amalagaura
Copy link

@forest I commented out this line and all tests still pass.

Is it possible to get a test case added that uses this Virtus patch so I can make sure I can replicate that behavior.

62 tests, 420 assertions, 0 failures, 0 errors, 0 skips

@amalagaura
Copy link

amalagaura commented Oct 4, 2019

OK it looks like that loading behavior is not behaving as expected because the patch is being loaded before Virtus::Attribute::NullifyBlank.method_defined?(:coerce) The require statements in the virtus gem are loading from the patch first.

Changing the patch file does result in a failed test

Minitest::Assertion: Expected [] to be nil.

@forest
Copy link
Contributor

forest commented Oct 28, 2019

@amalagaura I'm happy to take your recommendation and fix for this. I'm no longer actively using this gem and so don't have the time to dig deep on these kinds of issues. I'm happy to continue to support the community of users though.

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

No branches or pull requests

2 participants