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

Deprecate `update_attributes` and `update_attributes!` #31998

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
9 participants
@elebow
Copy link
Contributor

elebow commented Feb 15, 2018

update_attributes and update_attributes! have been aliases for update and update! since #8705 in 2013. That PR says it was a "soft deprecation" and that the longer method names would be removed in the future.

This would be a desirable change because:

  1. It makes application code easier to read by reducing confusion with update_attribute/update_attribute!, and
  2. The more direct names help new users understand that these are the "normal" ways to modify attributes of an Active Record object.

Is this still the intention? update_attributes is still used in a few tests and one place in ActiveRecord::InternalMetadata, and mentioned in the AR Callbacks guide; I can change them to the newer forms in this PR if that is the goal.

@rails-bot

This comment has been minimized.

Copy link

rails-bot commented Feb 15, 2018

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.

@sikachu

This comment has been minimized.

Copy link
Member

sikachu commented Feb 15, 2018

5 years ago, wow ...

r? @guilleiguaran

@rails-bot rails-bot assigned guilleiguaran and unassigned schneems Feb 15, 2018

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Feb 17, 2018

Yeah, I think it is time to deprecate them. Can you remove all the usages and add tests to check if those methods are deprecated?

@guilleiguaran
Copy link
Member

guilleiguaran left a comment

Please replace all the usages of update_attributes with update in the test suite

end
assert_not_nil error.cause
assert_not_equal "Hm is it possible?", Topic.find(3).title

topic.update_attributes(id: 1234)
topic.update(id: 1234)
assert_nothing_raised { topic.reload }
assert_equal topic.title, Topic.find(1234).title

This comment has been minimized.

Copy link
@elebow

elebow Feb 17, 2018

Author Contributor

Some of these cases were present in test_update_attributes but not in test_update. They are now moved to test_update.

@jeremy jeremy closed this in 5645149 Feb 17, 2018

@jeremy

This comment has been minimized.

Copy link
Member

jeremy commented Feb 17, 2018

Thanks @elebow!

@sgrif

This comment has been minimized.

Copy link
Contributor

sgrif commented Feb 17, 2018

Are these aliases causing some sort of maintenance burden? If we actually fully deprecate/remove these methods we are breaking a lot of code for virtually no reason. I'm all for keeping them soft-deprecated, but if they aren't causing a maintenance burden I see no reason to actually remove them.

@pat

This comment has been minimized.

Copy link
Contributor

pat commented Mar 2, 2019

Just chiming in to raise the same concerns as @sgrif - I think removing these methods could break a significant amount of code in so many gems, and that feels unwise to me.

I personally didn't know update was actually the preferred style until maybe early 2018 - there'd never been any warnings about update_attributes, so I know I've taken the 'outdated' approach for quite a long time, and I'm sure plenty of other gem developers have as well.

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