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

Turn options into indifferent access #16254

Merged
merged 1 commit into from
Oct 23, 2017

Conversation

bzwei
Copy link
Contributor

@bzwei bzwei commented Oct 20, 2017

The existing code expects all options keys are symbols. We discovered symbols keys can cause troubles for serializing and deserializing. With the simple fix we now accept either symbol or string.

@bzwei
Copy link
Contributor Author

bzwei commented Oct 20, 2017

@miq-bot add_label enhancement
@miq-bot assign @gmcculloug
@mkanoor please review

@gmcculloug
Copy link
Member

@mkanoor I think this is in response to the symbol/string issue in the automate Ansible Playbook method you were mentioning. Do we still need this?

@syncrou Any thoughts on this one?

Copy link
Contributor

@syncrou syncrou left a comment

Choose a reason for hiding this comment

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

This looks good to me. 👍

@gmcculloug
Copy link
Member

@bzwei Please add some tests around this change. If we start using strings or symbols we will need a test in case this inadvertently gets removed.

@bzwei bzwei force-pushed the options_with_indifferent_access branch from afb97c3 to efe4ee8 Compare October 23, 2017 14:13
@bzwei
Copy link
Contributor Author

bzwei commented Oct 23, 2017

@gmcculloug I changed one of the tests to use string keys. It will fail if we remove the indifferent accessibility.

@miq-bot
Copy link
Member

miq-bot commented Oct 23, 2017

Checked commit bzwei@efe4ee8 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@gmcculloug gmcculloug merged commit b80718d into ManageIQ:master Oct 23, 2017
@gmcculloug gmcculloug added this to the Sprint 72 Ending Oct 30, 2017 milestone Oct 23, 2017
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.

4 participants