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

Remove association override #36

Merged
merged 6 commits into from
Feb 20, 2024

Conversation

evman182
Copy link
Collaborator

No description provided.

@evman182 evman182 force-pushed the eraffel/FixAssociations branch from b870bb3 to ba12d0f Compare February 13, 2024 23:08
@evman182
Copy link
Collaborator Author

@bkroeker Do you remember why specifying the primary key was needed like this? There's no tests that are failing, and the current implementation is causing issues for me in Rails 7.1

@bkroeker
Copy link
Owner

@evman182 Your change makes sense to me. I can't think of what use case I would have been coding for here. If possible, and if there isn't one already, can you add a spec for a table which uses a non-standard primary key?

@evman182 evman182 changed the title Remove association overrides Remove association override Feb 14, 2024
@evman182 evman182 marked this pull request as ready for review February 14, 2024 20:55
@evman182
Copy link
Collaborator Author

evman182 commented Feb 14, 2024

@bkroeker what scares me is that the change was made 11 years ago, so I'm afraid of what it might break:
9d9ad8e#diff-41a31b560d905668bb22687bb704f31f6e84cd4eedc2e8395d21920a87e9a42fR37

@evman182
Copy link
Collaborator Author

@bkroeker Are you good with the tests here? Any remaining concerns?

Copy link
Owner

@bkroeker bkroeker left a comment

Choose a reason for hiding this comment

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

Approved with one naming suggestion. Thanks.

spec/basic_history_spec.rb Outdated Show resolved Hide resolved
@evman182 evman182 merged commit d502ba4 into bkroeker:master Feb 20, 2024
19 checks passed
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.

2 participants