-
Notifications
You must be signed in to change notification settings - Fork 21.8k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add ActiveModel::Model, a mixin to make Ruby objects to work with AP …
…inmediatly
- Loading branch information
1 parent
14f06dd
commit 3b822e9
Showing
4 changed files
with
44 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
module ActiveModel | ||
module Model | ||
def self.included(base) | ||
base.class_eval do | ||
extend ActiveModel::Naming | ||
extend ActiveModel::Translation | ||
include ActiveModel::Validations | ||
include ActiveModel::Conversion | ||
end | ||
end | ||
|
||
def initialize(params={}) | ||
params.each do |attr, value| | ||
self.send(:"#{attr}=", value) | ||
end if params | ||
end | ||
|
||
def persisted? | ||
false | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
require 'cases/helper' | ||
|
||
class ModelTest < ActiveModel::TestCase | ||
include ActiveModel::Lint::Tests | ||
|
||
class BasicModel | ||
include ActiveModel::Model | ||
attr_accessor :attr | ||
end | ||
|
||
def setup | ||
@model = BasicModel.new | ||
end | ||
|
||
def test_initialize_with_params | ||
object = BasicModel.new(:attr => "value") | ||
assert_equal object.attr, "value" | ||
end | ||
end |
3b822e9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
3b822e9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, this is missing docs. Can we please push some docs to ActiveModel::Model, explain how to use it and mention how other Active Model modules could be included?
3b822e9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice! Great job. Can we use
public_send
rather thansend
to set the attributes though?3b822e9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For further reference, docs were added and
send
is nowpublic_send
.@josevalim do you think we need more docs on that?
3b822e9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current docs look great. Thanks! <3
3b822e9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is an excellent direction for ActiveModel to be moving. But I'm afraid that including the mass assignment on initialization muddles the architecture of ActiveModel. What people are asking for is the simplest possible way to conform to the ActiveModel API, but by adding the initialize method you've gone a step further. I agree with wanting to provide something ActiveRecord-like, but this initialize method isn't ActiveRecord like. It doesn't incorporate mass assignment security (a hot topic lately), or the secondary option hash argument. It also isn't in line with ActiveModel's pick-and-choose library of modules approach.
What if initialize was removed and ActiveModel::Model was renamed to ActiveModel::BasicModel, the bare minimum to comply with the ActiveModel API, and then ActiveModel::Model can combine it and other modules, implementing no functionality itself, to give people the curated functionality people have come to expect from Rails.
3b822e9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cgriego I am not sure I see the issue given that a user can very easily overload
initialize
and implement their own functionality. Seems like extra ceremony to split behaviour into another module.Renaming it to
ActiveModel::Basic
would be nice though, IMO.3b822e9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I usually build AMo models is with an attributes hash and
read_attributes
delegating to it. That way I don't have to define accessors if I don't want to.