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

clean up and refactor active record container backend #543

Merged

Conversation

doits
Copy link
Contributor

@doits doits commented Nov 5, 2021

fixes dirty tracking and creating empty translation hashes on read

fixes #540

@shioyama
Copy link
Owner

shioyama commented Nov 8, 2021

Thanks for this! ❤️ Will review this week.

@doits doits force-pushed the fix_ar_container_backend_change_on_read branch 2 times, most recently from ace21a7 to e7e0b4d Compare December 2, 2021 16:24
@doits
Copy link
Contributor Author

doits commented Dec 2, 2021

I rebased everything and updated as requested:

  • Always assume model[column_name] is a hash
  • No compact anymore
  • One spec about removing empty locale hashes when last attribute is deleted

One thing to keep in mind is that empty hashes might already exist in saved models if the container backend was used before this PR and it will not "self heal" (like it did with compact). Not sure what to do about it though, or if there should be done anything about it.

Edit: I did it as extra commits so you can follow it. I can squash all commits if everything is good at the end.

@doits doits force-pushed the fix_ar_container_backend_change_on_read branch from e7e0b4d to b47131f Compare December 2, 2021 16:31
@doits doits requested a review from shioyama December 2, 2021 16:33
@doits doits force-pushed the fix_ar_container_backend_change_on_read branch from b47131f to b0613ae Compare December 2, 2021 16:40
@doits
Copy link
Contributor Author

doits commented Feb 27, 2022

Hey @shioyama, hope you're good! Any chance to review this PR again? I'd like to get this merged and off my list :-) If there are some changes needed I'll update the PR, just say so.

(Edit: Just rebased it on master)

@doits doits force-pushed the fix_ar_container_backend_change_on_read branch from b0613ae to 1991ddf Compare February 27, 2022 22:08
@shioyama shioyama merged commit d75c88d into shioyama:master Feb 28, 2022
@shioyama
Copy link
Owner

@doits Thanks so much, this is a really great PR! ❤️

One thing to keep in mind is that empty hashes might already exist in saved models if the container backend was used before this PR and it will not "self heal" (like it did with compact). Not sure what to do about it though, or if there should be done anything about it.

I think that's ok, if it hasn't been a problem so far then presumably it won't suddenly start being one (for those using the container backend).

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.

Backend container reader mixes up dirty tracking and creates empty locale hash
2 participants