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 the use of primary keys other than "id" in ActiveRecord. #237

Conversation

benk-gc
Copy link
Contributor

@benk-gc benk-gc commented Apr 1, 2024

Previously the diffing code made an assumption that "id" would always be the primary field. This is not always the case, and the use of read_attribute(:id) is deprecated in rails/rails@39997a0.

This changes adds support for use of custom primary key fields, and updates the Person test model to use the "person_id" primary key.

Note that this will almost certainly still run into issues if the model uses the newly introduced composite primary key type, so this would require additional changes outside the scope of this fix.

@benk-gc benk-gc force-pushed the benk--support-custom-primary-key-fields-activerecord branch from cb9b75c to 4695d54 Compare April 1, 2024 14:14
@benk-gc benk-gc marked this pull request as ready for review April 1, 2024 14:14
Previously the diffing code made an assumption that "id" would always be
the primary field. This is not always the case. Additionally, the use of
read_attribute(:id) is deprecated in rails/rails@39997a0.

This changes adds support for use of custom primary key fields, and
updates the Person test model to use the "person_id" primary key.

Note that this will almost certainly still run into issues if the model
uses the newly introduced composite primary key type, so this would
require additional changes outside the scope of this fix.
@benk-gc benk-gc force-pushed the benk--support-custom-primary-key-fields-activerecord branch from 4695d54 to 7ffc318 Compare April 1, 2024 15:26
@benk-gc
Copy link
Contributor Author

benk-gc commented Apr 4, 2024

@mcmire Any chance you could approve the test workflow and give this one a look? This would unblock us using the gem in Rails 7.1. Thanks!

@benk-gc
Copy link
Contributor Author

benk-gc commented Apr 10, 2024

@mcmire Specs looks happy - what do you think about a merge + release for this one? Happy to make any changes.

@benk-gc
Copy link
Contributor Author

benk-gc commented Apr 24, 2024

@mcmire Ping on the above when you have a sec - thanks!

Copy link
Owner

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Sorry about the delay in reviewing. Looks good. Thank you!

@mcmire mcmire merged commit 8b971a8 into mcmire:main Apr 25, 2024
31 checks passed
@mcmire
Copy link
Owner

mcmire commented Apr 25, 2024

I've released this under 0.12.0: https://github.com/mcmire/super_diff/releases/tag/v0.12.0. Thanks again!

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.

None yet

2 participants