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

Fix ActiveSupport 5.1 deprecation warnings #262

Merged
merged 1 commit into from
Apr 14, 2017
Merged

Fix ActiveSupport 5.1 deprecation warnings #262

merged 1 commit into from
Apr 14, 2017

Conversation

cpence
Copy link
Contributor

@cpence cpence commented Apr 11, 2017

Notably, there are still 4 test failures on ActiveRecord-edge, having to do with the "chained activerecord association subroot" tests. Debugging those is beyond my level of acquaintance with the code base. But the deprecation warnings surrounding attribute_changed?, attribute_was, and changes have all been silenced here.

@cpence
Copy link
Contributor Author

cpence commented Apr 12, 2017

Oddly, the failures that are showing up under Travis for the edge appraisal are not the same ones that I'm getting on my machine. But they're too tough for me to parse, and there's no deprecation warnings (other than those coming out of database_cleaner), so I think this PR is doing its job.

@@ -35,10 +35,13 @@ def _ct_before_save
end

def _ct_after_save
if changes[_ct.parent_column_name] || @was_new_record
as_5_1 = ActiveSupport.version >= Gem::Version.new('5.1.0')
changes_method = as_5_1 ? :saved_changes : :changes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow, that's pretty horrible that Rails is making us do this. Bummer.

I can't think of a more elegant way to do it though. Thanks for this PR.

Choose a reason for hiding this comment

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

Maybe this is a little less ugly?

changes_method = respond_to?(:saved_changes) ? :saved_changes : :changes

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, maybe? I guess I'm just frustrated by the rails team for making changes like this. Over and over again. If you think the other way is better, I'll merge your PR.

Copy link

Choose a reason for hiding this comment

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

I'm just frustrated by the rails team for making changes like this.

For the record, this change was not "oh this is better this way". The change in behavior of these methods was required to fix dozens of bugs, and significantly simplify the implementation of associations (weeding out even more bugs). It's not something that was taken lightly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sgrif not meant as a personal affront--just a recognition of seven years of API churn.

Copy link

Choose a reason for hiding this comment

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

For sure. There have been a lot of needless changes that hurt gems over the years.

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

Successfully merging this pull request may close these issues.

4 participants