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

Support Rails 5.2 #256

Merged
merged 4 commits into from
Apr 30, 2018
Merged

Support Rails 5.2 #256

merged 4 commits into from
Apr 30, 2018

Conversation

andrykonchin
Copy link
Member

@andrykonchin andrykonchin commented Apr 25, 2018

Related issue #254

@andrykonchin andrykonchin changed the title Support Rails 5.2.2 Support Rails 5.2 Apr 25, 2018
@coveralls
Copy link

coveralls commented Apr 25, 2018

Coverage Status

Coverage increased (+0.002%) to 97.601% when pulling 8deafbf on support-rails-5-2-2 into c007dec on master.

@coveralls
Copy link

coveralls commented Apr 25, 2018

Coverage Status

Coverage increased (+0.003%) to 97.603% when pulling bac743c on support-rails-5-2-2 into c007dec on master.

Copy link
Collaborator

@richardhsu richardhsu left a comment

Choose a reason for hiding this comment

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

Will test this to see if it addresses everything. Walking through the PR seems like they are still supporting the ivar structure of Dirty API so this will probably be fine until they decide to drastically change that.

@@ -5,7 +5,7 @@ module Dirty

module ClassMethods
def from_database(*)
super.tap { |d| d.changed_attributes.clear }
super.tap { |d| d.send(:clear_changes_information) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably need to have it handle both versions since older versions of rails I don't think will support this so maybe:

d.respond_to?(:clear_changes_information) ? d.clear_changes_information : d.changed_attributes.clear

And similar to below to handle both

Copy link
Member Author

Choose a reason for hiding this comment

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

@richardhsu fixed

@andrykonchin
Copy link
Member Author

andrykonchin commented Apr 26, 2018

@richardhsu I would consider independent implementation of changes tracking. Depending on Rails internals makes me sad.

@richardhsu
Copy link
Collaborator

@andrykonchin Yeah makes sense, and yeah their internals are changing so much now.

I've run a few tests with this update and looking good so far. Going to run full suite tomorrow and make sure those pass.

@nijikon
Copy link

nijikon commented Apr 30, 2018

I'm running this in production, so far no issues.

Copy link
Collaborator

@richardhsu richardhsu left a comment

Choose a reason for hiding this comment

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

LGTM

@andrykonchin andrykonchin merged commit 9bff6b6 into master Apr 30, 2018
@andrykonchin andrykonchin deleted the support-rails-5-2-2 branch April 30, 2018 21:09
@andrykonchin
Copy link
Member Author

@pboling I would like to release new version 2.1.1 with this bug fix only. Does it make sense?

@pboling
Copy link
Member

pboling commented May 1, 2018

@andrykonchin do it!

@pboling
Copy link
Member

pboling commented May 1, 2018

As for depending on Rails internals, I think making this gem a standalone ORM that can be used with other frameworks would be great. Might as well make that a target goal of a future release.

arvados-bot pushed a commit to arvados/arvados that referenced this pull request Aug 24, 2020
This was a tough one to diagnose. The Dirty module introduced the usage of an
attribute mutation tracker, being disabled here for functional tests to start
working again.
This commit is based on: Dynamoid/dynamoid#256

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas@di-pentima.com.ar>
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.

5 participants