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

Extracted attributes assingment from ActiveRecord to ActiveModel #10776

Merged
merged 1 commit into from Jan 23, 2015

Conversation

bogdan
Copy link
Contributor

@bogdan bogdan commented May 28, 2013

Now you are able to do:

class Feedback
  include ActiveModel::AttributeAssignment
  attr_accessor :email, :subject, :body
end

f = Feedback.new
f.assign_attributes(
  :email => "bogdan@example.com", 
  :subject => "Hello", 
  :body => "The app dont work for me"
)

This is just a proof of concept PR.

There is still some todos:

  • Write tests for ActiveModel::AttributeAssignment
  • Deprecate AR::UnknownAttributeError in flavor of AM one
  • Add change log entry
  • rebase change log entry 2-3 times

I am gonna add them if this will be approved to merge in.

end

end
UnknownAttributeError = ActiveModel::AttributeAssignment::UnknownAttributeError
Copy link
Contributor

@dmathieu dmathieu May 28, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is still necessary.

Copy link
Contributor Author

@bogdan bogdan May 28, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Existing rails apps could still use this constant to catch this exception and process it manually.

Copy link
Contributor

@dmathieu dmathieu May 28, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raising it has to trigger a deprecation then.

Copy link
Contributor Author

@bogdan bogdan Feb 13, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know how to do that?

@dmathieu
Copy link
Contributor

dmathieu commented May 28, 2013

Is there a reason why you didn't extract the tests too ?

@bogdan
Copy link
Contributor Author

bogdan commented Sep 25, 2013

@dmathieu once I saw someone from core team saying that overlapping between AR and AM tests is good idea. No concrete reason.


private

def _assign_attribute(k, v)
Copy link
Member

@robin850 robin850 Sep 28, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please indent the private methods as the contribution guideline states ? Also, I think that this will at least need a changelog entry. Not sure if you should add one in AR changelog as well.

Copy link
Member

@robin850 robin850 Sep 28, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice pull request though, thank you!

@rafaelfranca
Copy link
Member

rafaelfranca commented Jan 21, 2015

What is the motivation for this extraction?

@bogdan
Copy link
Contributor Author

bogdan commented Jan 21, 2015

Using outside of activerecord just like validation and others:

Attributes assignment concept is something I personally use a lot for many types of objects.

@rafaelfranca
Copy link
Member

rafaelfranca commented Jan 21, 2015

👍 for the idea from my side. @sgrif WDYT?

@sgrif
Copy link
Contributor

sgrif commented Jan 23, 2015

👍 for the idea from me, as well. Would like to review the code again once it's been rebased onto master and had tests added/moved/etc

@bogdan
Copy link
Contributor Author

bogdan commented Jan 23, 2015

Updated the PR:

  • Rebased with master
  • Added tests for ActiveModel
  • Decided to keep a separated documentation for #attributes= in activerecord because it includes many DB interaction points
  • Left ActiveRecord::AttributeAssignment tests untouched that now creates some overlap in tests. Thought it is a good idea
  • Made changelog

@arthurnn
Copy link
Member

arthurnn commented Jan 23, 2015

I would use this on Mongoid if this is merge on ActiveModel.!

_assign_attributes(sanitize_for_mass_assignment(attributes))
end

def _assign_attributes(attributes)
Copy link
Member

@rafaelfranca rafaelfranca Jan 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not this method be private?

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Jan 23, 2015
@bogdan
Copy link
Contributor Author

bogdan commented Jan 23, 2015

@rafaelfranca removed all whitespace

@@ -1,3 +1,23 @@
* Extracted `ActiveRecord::AttributeAssignment` to `ActiveModel::AttributesAssignment`
Copy link
Member

@vipulnsward vipulnsward Jan 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AttributesAssignment => AttributeAssignment

# cat = Cat.new
# cat.assign_attributes(name: "Gorby", status: "yawning")
# cat.name # => 'Gorby'
# cate.status => 'yawning'
Copy link
Member

@vipulnsward vipulnsward Jan 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

…utesAssignment`

Allows to use it for any object as an includable module.
@sgrif sgrif merged commit 2606fb3 into rails:master Jan 23, 2015
sgrif added a commit that referenced this pull request Jan 23, 2015
Minor style changes across the board. Changed an alias to an explicit
method declaration, since the alias will not be documented otherwise.
sgrif added a commit that referenced this pull request Jan 23, 2015
Extracted attributes assingment from ActiveRecord to ActiveModel
@egilburg
Copy link
Contributor

egilburg commented Jan 23, 2015

❤️

@egilburg
Copy link
Contributor

egilburg commented Jan 23, 2015

I wonder if, once all non-db-specific logic (including recent one such as the user-facing casting half) is extracted to AM, it would make sense to have a more full-featured out-of-the-box implementation of active model for those that want it to behave not just as a form object but as much as a full AR as possible, except not persisted. That means all the same validations/errors, attribute assignment, casting, etc.

Perhaps call it ActiveModel::Base which would be a richer implementation than ActiveModel::Model, possibly extending it.

end
UnknownAttributeError = ActiveSupport::Deprecation::DeprecatedConstantProxy.new( # :nodoc:
'ActiveRecord::UnknownAttributeError',
'ActiveModel::AttributeAssignment::UnknownAttributeError'
Copy link
Contributor

@egilburg egilburg Jan 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that end users may be rescuing from this error in their apps, it seems like a rather long and implementation-specific namespace. Perhaps keep it under ActiveModel::UnknownAttributeError?

Copy link
Member

@robin850 robin850 Jan 31, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I agree there ; this is too implementation-specific! 👍

Copy link
Contributor

@sgrif sgrif Jan 31, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, let's leave this undeprecated, and just alias it. It's an unnecessary change for users, and forces them to care too much about implementation.

Copy link
Contributor

@sgrif sgrif Jan 31, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robin850 Would you care to open a PR?

Copy link
Contributor Author

@bogdan bogdan Jan 31, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this undeprecated can cause confusion because while constant alias will work the caught exception instances will still have new name. It looks like people are just yelling about too long name space and want it to be sorter like in @egilburg's comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet