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

Remove safe_yaml gem in favour of using Psych which is in the stdlib #303

Merged
merged 11 commits into from
May 15, 2021

Conversation

ayushn21
Copy link
Member

@ayushn21 ayushn21 commented May 12, 2021

This is a 🐛 bug fix.

Summary

The safe_yaml gem doesn't seem like it's maintained any more. The Psych module which is part of the stdlib has a safe_load method so we can drop that external dependency and use the stdlib directly.

Context

Resolves #300

@jaredcwhite
Copy link
Member

This looks good! One question: is there a test to double-check it's really safe…like it can't instantiate a non-allowlisted object?

@ayushn21
Copy link
Member Author

is there a test to double-check it's really safe…like it can't instantiate a non-allowlisted object

I know it's safe because I ran into that error a number of times while working on this hahaha. But yeah definitely worth adding a test for this. Will write a couple of tests for Bridgetown::YAMLParser.

@ayushn21
Copy link
Member Author

Added some tests. Let me know what you think!

@KonnorRogers
Copy link
Member

@jaredcwhite heres some background on safe_load ruby/psych#487

@jaredcwhite
Copy link
Member

@ayushn21 Awssome! LGTM

@jaredcwhite jaredcwhite merged commit 9b459b1 into bridgetownrb:main May 15, 2021
@ayushn21 ayushn21 deleted the remove-safe-yaml branch May 15, 2021 16:39
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.

Drop safe_yaml dependency in favour of using Psych directly
3 participants