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

Suppress deprecation warnings of Psych.safe_load args in Ruby 2.6 #877

Closed
wants to merge 4 commits into from

Conversation

koic
Copy link

@koic koic commented Oct 24, 2018

The interface of Psych.safe_load will change from Ruby 2.6.
ruby/ruby@1c92766

This PR suppresses the following wargnins in Ruby 2.6.0-dev.

% cd path/to/codeclimate/repo
% ruby -v
ruby 2.6.0dev (2018-10-21 trunk 65252) [x86_64-darwin17]
% bundle exec rake

(snip)

warning: Passing whitelist_classes with the 2nd argument of Psych.safe_load is deprecated. Use keyword argument like Psych.safe_load(yaml, whitelist_classes: ...) instead.
warning: Passing whitelist_symbols with the 3rd argument of Psych.safe_load is deprecated. Use keyword argument like Psych.safe_load(yaml, whitelist_symbols: ...) instead.
warning: Passing aliases with the 4th argument of Psych.safe_load is deprecated. Use keyword argument like Psych.safe_load(yaml, aliases: ...) instead.
warning: Passing filename with the 5th argument of Psych.safe_load is deprecated. Use keyword argument like Psych.safe_load(yaml, filename: ...) instead.

(snip)

Failures:

  1) CC::CLI::VersionChecker prints nothing when up to date
     Failure/Error: expect(stderr).to eq ""

       expected: ""
            got: "warning: Passing whitelist_classes with the 2nd
            argument of Psych.safe_load is deprecated. Use
            keywo....safe_load is deprecated. Use keyword argument like
            Psych.safe_load(yaml, filename: ...) instead.\n"

(snip)

  2) CC::CLI::VersionChecker does nothing when API is unavailable
     Failure/Error: expect(stderr).to eq ""

       expected: ""
            got: "warning: Passing whitelist_classes with the 2nd
            argument of Psych.safe_load is deprecated. Use
            keywo....safe_load is deprecated. Use keyword argument like
            Psych.safe_load(yaml, filename: ...) instead.\n"

(snip)

  3) CC::CLI::VersionChecker does nothing if checked recently
     Failure/Error: expect(stderr).to eq ""

       expected: ""
            got: "warning: Passing whitelist_classes with the 2nd
            argument of Psych.safe_load is deprecated. Use
            keywo....safe_load is deprecated. Use keyword argument like
            Psych.safe_load(yaml, filename: ...) instead.\n"

(snip)

Finished in 0.63326 seconds (files took 0.71064 seconds to load)
338 examples, 3 failures

Failed examples:

rspec ./spec/cc/cli/version_checker_spec.rb:71 # CC::CLI::VersionChecker prints nothing when up to date
rspec ./spec/cc/cli/version_checker_spec.rb:81 # CC::CLI::VersionChecker does nothing when API is unavailable
rspec ./spec/cc/cli/version_checker_spec.rb:96 # CC::CLI::VersionChecker does nothing if checked recently

@CLAassistant
Copy link

CLAassistant commented Oct 24, 2018

CLA assistant check
All committers have signed the CLA.

@koic koic force-pushed the suppress_psych_safe_load_warnings branch 3 times, most recently from c87aea3 to fc9167c Compare October 24, 2018 05:36
@filipesperandio
Copy link
Contributor

Hey @koic the issue you mentioned wasn't properly linked, could you fix that so we get the full context?

The interface of `Psych.safe_load` will change from Ruby 2.6.
ruby/ruby@1c92766

This PR suppresses the following wargnins in Ruby 2.6.0-dev.

```console
% cd path/to/codeclimate/repo
% ruby -v
ruby 2.6.0dev (2018-10-21 trunk 65252) [x86_64-darwin17]
% bundle exec rake

(snip)

warning: Passing whitelist_classes with the 2nd argument of
Psych.safe_load is deprecated. Use keyword argument like
Psych.safe_load(yaml, whitelist_classes: ...) instead.
warning: Passing whitelist_symbols with the 3rd argument of
Psych.safe_load is deprecated. Use keyword argument like
Psych.safe_load(yaml, whitelist_symbols: ...) instead.
warning: Passing aliases with the 4th argument of Psych.safe_load is
deprecated. Use keyword argument like Psych.safe_load(yaml, aliases: ...) instead.
warning: Passing filename with the 5th argument of Psych.safe_load is
deprecated. Use keyword argument like Psych.safe_load(yaml, filename: ...) instead.

(snip)

Failures:

  1) CC::CLI::VersionChecker prints nothing when up to date
     Failure/Error: expect(stderr).to eq ""

       expected: ""
            got: "warning: Passing whitelist_classes with the 2nd
            argument of Psych.safe_load is deprecated. Use
            keywo....safe_load is deprecated. Use keyword argument like
            Psych.safe_load(yaml, filename: ...) instead.\n"

(snip)

  2) CC::CLI::VersionChecker does nothing when API is unavailable
     Failure/Error: expect(stderr).to eq ""

       expected: ""
            got: "warning: Passing whitelist_classes with the 2nd
            argument of Psych.safe_load is deprecated. Use
            keywo....safe_load is deprecated. Use keyword argument like
            Psych.safe_load(yaml, filename: ...) instead.\n"

(snip)

  3) CC::CLI::VersionChecker does nothing if checked recently
     Failure/Error: expect(stderr).to eq ""

       expected: ""
            got: "warning: Passing whitelist_classes with the 2nd
            argument of Psych.safe_load is deprecated. Use
            keywo....safe_load is deprecated. Use keyword argument like
            Psych.safe_load(yaml, filename: ...) instead.\n"

(snip)

Finished in 0.63326 seconds (files took 0.71064 seconds to load)
338 examples, 3 failures

Failed examples:

rspec ./spec/cc/cli/version_checker_spec.rb:71 # CC::CLI::VersionChecker
prints nothing when up to date
rspec ./spec/cc/cli/version_checker_spec.rb:81 # CC::CLI::VersionChecker
does nothing when API is unavailable
rspec ./spec/cc/cli/version_checker_spec.rb:96 # CC::CLI::VersionChecker
does nothing if checked recently
```
@koic koic force-pushed the suppress_psych_safe_load_warnings branch from fc9167c to 2f1b0cc Compare October 25, 2018 03:13
@koic
Copy link
Author

koic commented Oct 25, 2018

@filipesperandio Oops, I'm sorry about it. There is no related issue. That is my mistake and I removed it. Thanks for your review.

@filipesperandio
Copy link
Contributor

filipesperandio commented Nov 9, 2018

@koic Do you usually do testing/dev outside of the Docker context? I am looking at this and feeling this is an unnecessary change once we do all testing within docker.

@wfleming WDYT?

@wfleming
Copy link
Contributor

wfleming commented Nov 9, 2018

I suspect @koic may be using the gem as a library within some other tool, hence wanting compatibility with Ruby 2.6? Is that accurate Koichi?

We discourage usage of the gem as a library because it's not a stable interface, and we do sometimes change it for internal reasons & only distribute as a gem at all to support some internal usage. We really should have distributed it as a private gem instead of a public one to avoid this confusion, and I'm sorry we didn't. I realize this can be confusing, so I apologize for that.

That being said, this is a small patch that seems entirely reasonable to me. I'll reiterate that usage of the gem as a library is not something we encourage people to do & I can't promise we'll always be able to accommodate changes to support those use cases, but if I'm right in my guess about why @koic submitted this change, it LGTM if it looks good to you, @filipesperandio.

@koic
Copy link
Author

koic commented Nov 12, 2018

I suspect @koic may be using the gem as a library within some other tool, hence wanting compatibility with Ruby 2.6? Is that accurate Koichi?

Yes. That's right. I appreciate your explanation. Actually this Psych's API has been changed later.

I will close this PR.

Code Climate is extremely grateful to help many OSS communities and others. Thank you very much for your team activities.

@koic koic closed this Nov 12, 2018
@koic koic deleted the suppress_psych_safe_load_warnings branch November 12, 2018 19:30
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.

4 participants