search

irphilli / tracker_api

Ruby Wrapper for Pivotal Tracker v5 API
MIT License
107 stars 109 forks source link

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

Open ghost opened 5 years ago

ghost commented 5 years ago

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: https://github.com/dashofcode/tracker_api/commit/27599e7e2169776c32bbff8c972a31b930452879#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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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.