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

Support Psych version 4 #1338

Merged
merged 1 commit into from
Sep 15, 2021
Merged

Conversation

t27duck
Copy link
Contributor

@t27duck t27duck commented Sep 5, 2021

Fixes #1335

Psych's .load method uses .safe_load by default which is not compatible with papertail needs to load.

This implements the suggested fix in the issue which is a similar fix Rails uses throughout its codebase when it needs to load YAML content.

The test case outlined in the issue passes with this change (after I pointed the paperclip gem to the forked branch).

Check the following boxes:

  • Wrote good commit messages.
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new
    code introduces user-observable changes.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.

@gurgelrenan
Copy link
Member

gurgelrenan commented Sep 15, 2021

Hi @t27duck , thanks for your P.R. Looks good to me.

@jaredbeck do you think that they should add some additional tests? I can't figure out a good way to test this cenario. Perhaps some mock in spec/paper_trail/serializers/yaml_spec.rb?

@jaredbeck
Copy link
Member

@jaredbeck do you think that they should add some additional tests? I can't figure out a good way to test this cenario. Perhaps some mock in spec/paper_trail/serializers/yaml_spec.rb?

Yes, a mock should work, e.g.

allow(YAML).to receive(:unsafe_load)
serializer.load
expect(YAML).to have_received(:unsafe_load)

@gurgelrenan
Copy link
Member

@t27duck Could you add the required tests?

Fixes paper-trail-gem#1335

Psych's `.load` method uses `.safe_load` by default which is not compatible with papertail needs to load.

This implements the suggested fix in the issue which is a similar fix that Rails uses throughout its codebase when it needs to load YAML content.
@t27duck
Copy link
Contributor Author

t27duck commented Sep 15, 2021

@gurgelrenan - Spec added.

Rspec doesn't allow stubbing methods that don't exist on the object that's being stubbed, so based on whichever version of Psych is running with the tests, I had to have the spec check what YAML does implement and set stubs and expectations based on that.

@jaredbeck
Copy link
Member

Thanks Tony. This is good enough.

Rspec doesn't allow stubbing methods that don't exist on the object that's being stubbed, ..

For future reference, I wonder if without_partial_double_verification would help ..? I haven't tried.

@jaredbeck jaredbeck merged commit 7e50b84 into paper-trail-gem:master Sep 15, 2021
@gurgelrenan
Copy link
Member

Thanks @t27duck 😄

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.

Support for Psych version 4
3 participants