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

don't assume a reflect_on_association method #499

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jrochkind
Copy link

I'm experimenting with using cocoon in a context where some of the models involved are ActiveModels rather than ActiveRecord::Base. For my work with this gem.

It actually works out pretty well, most parts of cocoon don't assume that the collections involved actually have AR reflection info, for instance. Perhaps some work has gone in this direction before!

There are just a couple places where cocoon is assuming methods exist where I'd rather it didn't.

This is one of them, which seems pretty straightforward to do something about -- it was accepting a nil return value from reflect_on_association anyway (and doing the right thing for my use case in that situation), so just have it not try to call the method if it doesn't exist, and just go with the nil value. Seems quite clean and neat.

What do you think?

@danbernier
Copy link

@jrochkind I don't know if you're still working on this, but if you have any other notes on getting cocoon to work with ActiveModels, I'm bashing my way through it now.

@jrochkind
Copy link
Author

jrochkind commented Feb 25, 2021

@danbernier I don't remember the details, but I did get cocoon working for my use cases in attr_json.

Basically, every time I ran into cocoon assuming a certain method was there, I went and looked at AR to see what the method did, and then just implemented it to do the "right" thing for my non-AR models. It was only a few methods. It was over two years ago, I don't remember all the details, but you can try searching for PR's or commits that mention cocoon.

Oh wait, look! As I look at my code, I see I extracted some stuff into a module, I don't know if it will meet your needs, but there's this: https://github.com/jrochkind/attr_json/blob/master/lib/attr_json/model/cocoon_compat.rb

Unfortunately it looks like the only spec I have that exersizes my cocoon compatibility is this kind of monstrous not well factored one: https://github.com/jrochkind/attr_json/blob/master/spec/integration/form_mega_spec.rb

@danbernier
Copy link

@jrochkind thank you! That's helpful. I've followed in your footsteps and came to a similar place, which is reassuring. If I bump into anything else, I'll note it here.

@jrochkind
Copy link
Author

jrochkind commented Feb 25, 2021

Since cocoon works fine, I have found, if you just return nil or false from those methods, it would be easy to PR cocoon to check respond_to? before calling them, and just follow the nil/false path if not.

but it's unclear to me if theres anyone maintaining cocoon who would be interested in entertaining such a PR. Based on this PR sitting here without comment for two years, apparently not.

gael-ian added a commit to notus-sh/cocooned that referenced this pull request Dec 11, 2022
gael-ian added a commit to notus-sh/cocooned that referenced this pull request Mar 7, 2023
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 this pull request may close these issues.

2 participants