-
Notifications
You must be signed in to change notification settings - Fork 808
[WIP] Convert from Virtus to ActiveAttr #395
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
Conversation
@ryansch I like the idea itself -- many people seem to have performance and other issues with Virtus.
What I'm thinking though, is that it would be best to make this support pluggable, in the spirit of "adapters" we have in elasticsearch-model for ActiveRecord, Mongoid, etc etc etc.
My reasoning would not be of academical, but of practical nature: so we rip out Virtus, add ActiveAttr, then after some time something better comes, we again change the code, etc etc etc. Can you give it some thinking and maybe play with it a bit?
I was wondering the same thing. Honestly the contract we have with the model is pretty thin so making this pluggable would be pretty easy.
e667404 to
e9fc24a
Compare
I've paired this down to just the active_attr stuff. I'll try to add the plugin architecture in the next couple weeks.
That would be awesome! Take your time and please ping me whenever you'd like to get some feedback or ping-pong something.
c202b4c to
3d2dd66
Compare
Just FYI, I haven't forgotten about this.
@ryansch, that's great to hear :) Take your time!
gmile
commented
Jan 5, 2016
@ryansch it looks like original author of virtus himself is abandoning the project. So virtus issues you're seeing are unlikely to be ever resolved, unless someone takes over the project. Yet even in that case I doubt virtus will remain a good choise, for reasons he well explained in this thread on reddit.
It's been a year since last commit in active_attr, maybe it's worth considering an alternative to active_attr as a default attrs lib, like the dry-data? What do you think?
I think I'd still like to add a pluggable interface so you can use whatever you want. :-)
We'll just have to decide what the default is.
Edit: By the author's own admission dry-data isn't production ready yet.
Edit2: This is still in the pipeline for me. I'm hoping to get to it in the next couple months.
gmile
commented
Jan 5, 2016
by the author's own admission dry-data isn't production ready yet
@ryansch you're right.
I'm just thinking outloud. active_attr depends on both activemodel and activesupport, and I'm a little biased towards both (those dependencies feel a bit too heavy for the task). But that's just me.
Upd. Realized elasticsearch-persistence depends on activemodel and activesupport too... So they hardly can be used as an argument against active_attr. Oh well. Please, ignore my comment above!
I think I'd still like to add a pluggable interface so you can use whatever you want. :-)
+++ :)
(It's just a question how much benefit would be in abstracting everything away like that...)
3d2dd66 to
c05022b
Compare
0c70d25 to
c05022b
Compare
I'm going to close this, as it's no longer relevant with the removal of Elasticsearch::Persistence::Model.
🎉
I've had so many issues with the way Virtus decides to do things that I finally gave up and ripped it out in favor of ActiveAttr.