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

Move the responsibility of safe loading to Node #643

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

michaelherold
Copy link

This change moves the responsibility of safely loading a YAML tree from an internal detail of Psych.safe_load into Psych::Nodes::Node.

The advantage of this is that you can now more easily safely load trees that you're working on without replicating the safety constraints of Psych.safe_load. The delegation of responsibilities also now matches for safely and unsafely loading a tree.

I opted for two choices to make the diff smaller:

  1. I left all of the documentation on Psych.safe_load instead of moving it to Psych::Nodes::Node#to_safe_ruby. The main reason I chose to do so is that Psych.safe_load is the most likely entry point into using Psych for most people, so having the documentation on that method makes sense.
  2. I delegated responsibility for the default arguments to Psych::Nodes::Node rather than duplicating them. The reason I chose to do this was to reduce the chance of two lists of arguments becoming unsynchronized. Additionally, since the behavior actually belongs to Psych::Nodes::Node now, it's the only place that can have the defaults set for all use cases.

This change moves the responsibility of safely loading a YAML tree from
an internal detail of `Psych.safe_load` into `Psych::Nodes::Node`.

The advantage of this is that you can now more easily safely load trees
that you're working on without replicating the safety constraints of
`Psych.safe_load`. The delegation of responsibilities also now matches
for safely and unsafely loading a tree.
Copy link
Member

@tenderlove tenderlove left a comment

Choose a reason for hiding this comment

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

This seems fine, but could you please add tests? I don't think we need to cover the entirety of the API (since I think safe_load tests do that), but something to make sure this API doesn't break.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants