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

Let ActiveRecord handle serializing settings as HashWithIndifferentAccess #16593

Closed
wants to merge 1 commit into from

Conversation

bdunne
Copy link
Member

@bdunne bdunne commented Dec 4, 2017

Require ManageIQ/manageiq-schema#141

Followup to #16572

@miq-bot
Copy link
Member

miq-bot commented Dec 4, 2017

Checked commit bdunne@3abda3c with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

Copy link
Contributor

@imtayadeway imtayadeway left a comment

Choose a reason for hiding this comment

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

I'm going to 👍 this but I do have some reservations that I want to note:

In c0a662b I was acknowledging that my fix was not entirely satisfactory to me. The much more extensive fix that I wanted was for all settings to be keyed with strings, allowing for a much more natural serialization/deserialization to/from JSON, and fewer surprises.

The bigger picture is that this is a general problem with any serialized column, which are used extensively, and indeed bugs like this tend to crop up wherever we use them. What I'd really like is a general solution to this problem that we can apply (eventually) everywhere, aiming for consistency. With that in mind I think an extensive use of HashWithIndifferentAccess would be regrettable, if using regular hashes keyed with strings was satisfactory. But I'm also acknowledging that this would be more work, and more prone to breaking things.

@bdunne
Copy link
Member Author

bdunne commented Feb 19, 2018

Change of opinion. I don't think this hash should contain string keys at all, closing.

@bdunne bdunne closed this Feb 19, 2018
@bdunne bdunne deleted the MiqGroup_settings_tech_debt branch February 19, 2018 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants