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.

Sign up for GitHub

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

Jump to bottom

Use friendlier terminology in YAML.safe_load #378

Merged
merged 1 commit into from
Nov 10, 2018
Merged

Use friendlier terminology in YAML.safe_load #378

merged 1 commit into from
Nov 10, 2018

Conversation

JuanitoFatas
Copy link
Member

@JuanitoFatas JuanitoFatas commented Nov 8, 2018 •
edited
Loading

This Pull Request deprecates the usage of whitelist. Prefer Permitted over Whitelist where applicable.

Permitted is easier to understand and better terminology.

Original motivation, community efforts examples:

sorryeh
sorryeh approved these changes Nov 9, 2018
View reviewed changes
Copy link
Contributor

@sorryeh sorryeh 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 not sure of the psych/ruby community stance on this type of change, but I'm all for it 👍 🚢

This was referenced Nov 9, 2018
Closed
Closed
Merged
@yuki24
Copy link
Member

yuki24 commented Nov 9, 2018 •
edited
Loading

There should be a deprecation warning as this would introduce a breaking change.

@sorryeh
Copy link
Contributor

sorryeh commented Nov 9, 2018

@yuki24 Seems like there is

https://github.com/ruby/psych/pull/378/files#diff-fcdbfb11714f576f58ba9f866052bc79R338

Line 338 to ~345

@yuki24
Copy link
Member

yuki24 commented Nov 9, 2018

@sorryeh Nice! Sorry I totally missed it.

@ioquatix
Copy link
Member

ioquatix commented Nov 9, 2018 •
edited
Loading

While I agree with the motivation for the change, I'm not sure I agree that changing a public API is appropriate in this case, even with deprecations. That doesn't mean I don't want to merge it, just that I'm very mindful of the upstream consequences, which include more warnings, additional busy work for any project using these arguments, etc.

I personally think permitted_classes is way better (as in technically more descriptive) than whitelist_classes.

@amatsuda
Copy link
Member

Basically I have no strong opinion concerning this kind of topic, but as for this particular issue, I'm +1 to change the API now. No deprecation warning is needed.
Because the kwargs API was not included in the latest stable psych gem release, 3.0.3: https://github.com/ruby/psych/blob/v3.0.3/lib/psych.rb#L318
It's only been a part of beta releases of the next minor.
No real user will be affected by the API change.

@ioquatix
Copy link
Member

Okay all my concerns are addressed and I think it’s a good change.

Use friendlier terminology in YAML.safe_load
682abf2 
Replace keyword argumment whitelist_classes and whitelist_symbols.
with permitted_classes and permitted_symbols.
This was referenced Nov 10, 2018
Merged
Closed
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Closed
Merged
Closed
@hsbt hsbt merged commit a6f8972 into ruby:master Nov 10, 2018
matzbot pushed a commit to ruby/ruby that referenced this pull request Nov 10, 2018
@hsbt
Merge psych from upstream.
db3101f 
  * ruby/psych#378

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@65656 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
@JuanitoFatas JuanitoFatas deleted the update_terminology branch November 11, 2018 10:17
koic added a commit to koic/rubocop that referenced this pull request Nov 12, 2018
@koic
Change kwarg names of Psych in Ruby 2.6
6360983 
This PR fixes the follwoing error.

```console
% ruby -v
ruby 2.6.0dev (2018-11-11 trunk 65667) [x86_64-darwin17]
% bundle exec rake
Files:         514
Modules:        80 (   11 undocumented)
Classes:       485 (    3 undocumented)
Constants:     751 (  738 undocumented)
Attributes:     26 (    0 undocumented)
Methods:      1163 ( 1037 undocumented)
 28.58% documented
 rake aborted!
 ArgumentError: unknown keywords: whitelist_classes, whitelist_symbols
 /Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/config_loader.rb:189:in `yaml_safe_load'
 /Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/config_loader.rb:162:in `load_yaml_configuration'
 /Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/config_loader.rb:41:in `load_file'
 /Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/config_loader.rb:107:in `default_configuration'
 tasks/cops_documentation.rake:247:in `main'
 tasks/cops_documentation.rake:262:in `block in <top (required)>'
 /Users/koic/.rbenv/versions/2.6.0-dev/bin/bundle:30:in `block in <main>'
 /Users/koic/.rbenv/versions/2.6.0-dev/bin/bundle:22:in `<main>'
 Tasks: TOP => default => generate_cops_documentation
 (See full trace by running task with --trace)
 ```

These kwarg names has been changed by the following links.

- ruby/psych#378
- ruby/ruby@6268098

For these kwarg names change, This PR will not add the new conditional branch
using `Gem::Version.new(Psych::VERSION)`. It is the same as for the following reasons.

> Because the kwargs API was not included in the latest stable psych gemrelease, 3.0.3:
https://github.com/ruby/psych/blob/v3.0.3/lib/psych.rb#L318
> It's only been a part of beta releases of the next minor.
> No real user will be affected by the API change.

ruby/psych#378 (comment)
@koic koic mentioned this pull request Nov 12, 2018
Merged
8 tasks
bbatsov pushed a commit to rubocop/rubocop that referenced this pull request Nov 12, 2018
@koic @bbatsov
Change kwarg names of Psych in Ruby 2.6
4910866 
This PR fixes the follwoing error.

```console
% ruby -v
ruby 2.6.0dev (2018-11-11 trunk 65667) [x86_64-darwin17]
% bundle exec rake
Files:         514
Modules:        80 (   11 undocumented)
Classes:       485 (    3 undocumented)
Constants:     751 (  738 undocumented)
Attributes:     26 (    0 undocumented)
Methods:      1163 ( 1037 undocumented)
 28.58% documented
 rake aborted!
 ArgumentError: unknown keywords: whitelist_classes, whitelist_symbols
 /Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/config_loader.rb:189:in `yaml_safe_load'
 /Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/config_loader.rb:162:in `load_yaml_configuration'
 /Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/config_loader.rb:41:in `load_file'
 /Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/config_loader.rb:107:in `default_configuration'
 tasks/cops_documentation.rake:247:in `main'
 tasks/cops_documentation.rake:262:in `block in <top (required)>'
 /Users/koic/.rbenv/versions/2.6.0-dev/bin/bundle:30:in `block in <main>'
 /Users/koic/.rbenv/versions/2.6.0-dev/bin/bundle:22:in `<main>'
 Tasks: TOP => default => generate_cops_documentation
 (See full trace by running task with --trace)
 ```

These kwarg names has been changed by the following links.

- ruby/psych#378
- ruby/ruby@6268098

For these kwarg names change, This PR will not add the new conditional branch
using `Gem::Version.new(Psych::VERSION)`. It is the same as for the following reasons.

> Because the kwargs API was not included in the latest stable psych gemrelease, 3.0.3:
https://github.com/ruby/psych/blob/v3.0.3/lib/psych.rb#L318
> It's only been a part of beta releases of the next minor.
> No real user will be affected by the API change.

ruby/psych#378 (comment)
@koic koic mentioned this pull request Nov 12, 2018
Closed
koic added a commit to koic/ridgepole that referenced this pull request Nov 13, 2018
@koic
Change kwarg names of Psych in Ruby 2.6
2d07ed3 
Some kwarg names of `Psych.safe_load` method has been changed by the following links.

- ruby/psych#378
- ruby/ruby@6268098
@koic koic mentioned this pull request Nov 13, 2018
Merged
@koic koic mentioned this pull request Nov 24, 2018
Closed
@JuanitoFatas JuanitoFatas mentioned this pull request Mar 21, 2019
Merged
@JuanitoFatas JuanitoFatas mentioned this pull request Apr 1, 2019
Closed
voxik added a commit to voxik/crack that referenced this pull request Mar 11, 2022
@voxik
Use named parameters for safe_load.
7fa8d8a 
This is available since Psych 3.1 [[1], [2]], but mandatory since Psych
4.0 [[3]].

Fixes jnunemaker#72

[1]: ruby/psych#358
[2]: ruby/psych#378
[3]: ruby/psych@0767227
@voxik voxik mentioned this pull request Mar 11, 2022
Closed
stanhu added a commit to stanhu/kramdown that referenced this pull request Mar 13, 2023
@stanhu
Fix deprecated interface for YAML.safe_load
e8f25f2 
Passing in the permitted classes as the second argument was deprecated
in Psych 3 (ruby/psych#378) and removed in
Psych 4 (ruby/psych#487).

Ruby 3.1 ships with Psych 4, so to make the code work for Ruby 2.7 and
up, use the `permitted_classes` keyword argument.
@stanhu stanhu mentioned this pull request Mar 13, 2023
Closed
@dmorehouse dmorehouse mentioned this pull request Aug 22, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorryeh sorryeh sorryeh approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@JuanitoFatas @yuki24 @sorryeh @ioquatix @amatsuda @hsbt