-
Notifications
You must be signed in to change notification settings - Fork 10
replace_virtual_field condenses returning includes #149
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
Conversation
|
update:
|
|
this is now part of #133 - we can merge here or there |
d2b4a45 to
4d62cba
Compare
|
update:
update:
|
|
@kbrock I'm not sure what this fixes. It looks like it fixes a bug but can't envision how this occurs.
Can you describe the problem you had here? |
Merged_includes via include_to_hash already promotes classes up to a Hash. But there was no code that demoted classes back to an array or symbol / nil. If you look at the changed specs, you can see how this reduces the values. In the rails 7.0 migration, I was going to add special code to introduce this but it seems best to do this at the source
|
Hey @jrafanie ok, I added a pre-commit to this PR. This over optimizes the list for rails 7, and that is why I added the |
|
Checked commits kbrock/activerecord-virtual_attributes@153a10f~...a1d7249 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint |
jrafanie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let's get this in and tested with our rails 7 work.
relace_virtual_fieldsometimes returns unnecessary tiers of embedded hashes or arrays. E.g.[[:field]]instead of:field.6.1 has one fixes, but is by no means exhaustive:
This fixes
replace_virtual_fieldto not output that value. Since this is at the return of each level, it essentially applied recursively and flattens it significantly.Added
freezein tests to verify that the input parameter is unmodified.