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

Allow options on Mash#load #474

Merged
merged 8 commits into from
Mar 22, 2019

Conversation

dblock
Copy link
Member

@dblock dblock commented Mar 22, 2019

A replacement for #473. The only users affected by this change would be those that have rolled a custom parser, vs. #473 that affects everyone who calls Mash#load.

  • Preserves backwards compatibility and doesn't change the behavior of Mash#load.
  • Allows passing options through, so one can Mash.load(file, whitelist_classes: [Symbol]) which was the desired effect of Enable load symbol class #473, and can specify other options such as aliases.
  • Documents the feature and the interface change for custom parsers.

@dblock
Copy link
Member Author

dblock commented Mar 22, 2019

@michaelherold Care to CR/squash/merge? Thanks.

@dblock dblock mentioned this pull request Mar 22, 2019
@dblock dblock force-pushed the blabo-feature/enable_load_symbol branch 2 times, most recently from 9d7fbc1 to 83cc64c Compare March 22, 2019 15:30
@dblock dblock force-pushed the blabo-feature/enable_load_symbol branch from 83cc64c to 01b511b Compare March 22, 2019 15:31
@dblock dblock force-pushed the blabo-feature/enable_load_symbol branch from 74a1621 to 6b90f31 Compare March 22, 2019 16:00
@dblock dblock changed the title A replacement for #473 Allow options on Mash#load Mar 22, 2019
Copy link
Member

@michaelherold michaelherold 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 good with this. The tests for the Psych::DisallowedClass were a little unclear but that's due to the nature of the stdlib implementation. I can't fault you for that!

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