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

implement ActiveRecord-like attributes API for ActiveModel #26728

Closed
wants to merge 1 commit into from

Conversation

@Azdaroth
Copy link
Contributor

commented Oct 6, 2016

Attributes API for ActiveRecord is absolutely great, I was wondering what would it take to make something similiar available for ActiveModel, so I wrote minimal implementation to handle the most essential features. Is it something that you think should be available in ActiveModel? If yes, should it be initially a minimal implementation that would evolve into something more sophisticated (that would mimic parts like ActiveRecord::Attribute) or should it be like that from the start?

Let me know if this feature makes sense, will add some docs and Changelog note if it's something desired.

@rails-bot

This comment has been minimized.

Copy link

commented Oct 6, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@maclover7

This comment has been minimized.

Copy link
Member

commented Oct 6, 2016

r? @sgrif

@rails-bot rails-bot assigned sgrif and unassigned schneems Oct 6, 2016
@Azdaroth

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2016

@sgrif Thoughts?

@sgrif

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2016

This is the direction that I'd eventually like to go, but I think this jumps the shark a bit, and the final Active Model integration needs to be a bit more... integrated. For example, I'd expect this API to properly interface with ActiveModel::Dirty, which probably requires deprecating the existing form of ActiveModel::AttributeMethods and ActiveModel::AttributeAssignment in favor of this one.

@Azdaroth

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2016

@sgrif Yeah, it definitely makes sense to make it work properly with the rest of the parts of ActiveModel. So if it worked with ActiveModel::Dirty then this implementation would be just fine? How would you expect the interface to look like to be more integrated? Something like this?:

include ActiveModel::Dirty
attribute :name, :string

define_attribute_methods :name

def name=(val)
  name_will_change! unless val == @name
  # I guess this is the problematic part as this should assign deserialized value, not the passed value
  super
end

I would be happy to extend it, I think this feature is really awesome and plenty of people could benefit from it, if you could provide some more details how the integration would ideally look like and what else should be added here, it would be totally aweome. Thanks for looking into it!

@sgrif

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2016

Basically the integration would mean moving most of the code from ActiveRecord::Dirty and ActiveRecord::AttributeAssignment up to active model. If you look at dirty in particular, there's a lot of duplication because I've been slowly in the process of pushing more behavior into Attribute, AttributeSet, and MutationTracker. Those changes haven't been able to live on active model though because Attribute and AttributeSet don't live there. So I would like to remove the setting of ivars entirely on Active Model, so that more of that behavior can be shared. It's especially tricky, because we need to go through some sort of deprecation cycle, but it's impossible to detect if someone is relying on the old behavior.

@Azdaroth

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2016

Thanks for the overview. No idea for now how the deprecations could be handled nicely, but I'll check the logic in ActiveRecord and see what could be moved to ActiveModel.

@Azdaroth

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2016

I should find some time next week to check it further, if anyone is interested in having it in the app, I've just released it as a gem: https://github.com/Azdaroth/active_model_attributes .

@sgrif

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2016

Glad to hear it. As mentioned, I think the implementation internally will need to be a good bit more involved than that, but I think having that gem is an excellent idea in the short to medium term. I'm going to close this, as I don't think we'll be moving forward with this implementation internally, but I appreciate the work you've done, as well as packaging it as a gem.

@sgrif sgrif closed this Dec 1, 2016
@Azdaroth Azdaroth referenced this pull request Apr 17, 2017
@jrochkind

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

I wrote something with some relation to this functionality, but specifically focused on embedding typed ActiveModel instances in a serialized json column in an ActiveRecord model -- including complex nested/compounded object graphs, basically a json(b) column as a simple document/object store.

Currently an in progress experiment, but it seems to working out well -- thanks so much @sgrif for the excellent foundation in ActiveModel::Type and related API that made it feasible to keep going in the direction I did.

https://github.com/jrochkind/json_attribute

I think it takes this idea closer to being something that could eventually be an internal implementation upstream here, although to keep myself sane I kept a focus on my specific use cases involving json serialization -- it was confusingly abstract enough even with that focus. But I think some of the pieces are in the right direction, if anyone's curious.

Would love some peer-review/feedback (and what the name should be; I don't feel right using the 'active' prefix without it being a rails team effort, but there aren't a lot of good names left!)

@jrochkind

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2018

@sgrif has an attributes-typing API made it into ActiveModel in 5.2?

@kaspth

This comment has been minimized.

Copy link
Member

commented Feb 25, 2018

@jrochkind this might be of interest #30985 😄

@jrochkind

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2018

Thanks @kaspth that's exactly what I was looking for. Indeed in all 5.2.0 pre releases, which I check by looking at the github UI divorced from the PR: c3675f5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.