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 arguments of has_many method #306

Merged
merged 2 commits into from
May 3, 2024

Conversation

sontixyou
Copy link
Contributor

@sontixyou sontixyou commented Apr 30, 2024

Problem being solved

fix this issue. #305

I think that merging this PR will eliminate the error that is occurring in the above issue.

Investigation

ActiveHash supports up to Rails ver5.0.
I checked Rails ver5.0 to Rails ver7.1
Then I found that the argument structure of the has_many method is almost the same.

Hence, I modified it to pass the same arguments as ActiveRecord#has_many.

@flavorjones
Copy link
Collaborator

flavorjones commented Apr 30, 2024

@sontixyou Thank you for submitting this. I would like to see a test for this usage, perhaps we can take the example @issei-m provided in #305?

@sontixyou
Copy link
Contributor Author

@flavorjones
Thanks for the reply!
I have added a test on the changes.
Could you please review them?

Copy link

@issei-m issei-m left a comment

Choose a reason for hiding this comment

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

Once applied this patch, my issue described in #305 was resolved!

@flavorjones
Copy link
Collaborator

Looks great, thank you so much!

@flavorjones flavorjones merged commit da9eeee into active-hash:master May 3, 2024
16 checks passed
@flavorjones
Copy link
Collaborator

@kbrock We should probably ship this in a bugfix release. You OK if I go ahead and do that?

@flavorjones
Copy link
Collaborator

Shipped in https://github.com/active-hash/active_hash/releases/tag/v3.3.1

@sontixyou sontixyou deleted the fix-has_many-method branch May 6, 2024 02:27
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.

3 participants