-
-
Notifications
You must be signed in to change notification settings - Fork 396
[Ruby 3.4] Add spec for GC.config #1306
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
Conversation
6be2052 to
0bd6e6e
Compare
Includes generic tests and tests for MRI's default implementation of GC.
If a Ruby implementation does not want to expose implementation or configuration, a minimal method can be this:
```ruby
def GC.config(options = nil)
return { implementation: "none" } if options.nil?
raise ArgumentError unless Hash === options
if options.key?(:implementation)
raise ArgumentError, 'Attempting to set read-only key "Implementation"'
end
{}
end
```
0bd6e6e to
ab5202d
Compare
core/gc/config_spec.rb
Outdated
| -> { GC.config([]) }.should raise_error(ArgumentError, "ArgumentError") | ||
| -> { GC.config("default") }.should raise_error(ArgumentError, "ArgumentError") | ||
| -> { GC.config(1) }.should raise_error(ArgumentError, "ArgumentError") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if specifying the message makes sense if there is no message. Maybe this is overspecification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's better to not specify the message if it's empty/missing or clearly wrong, could you remove those?
|
|
||
| it "includes the name of currently loaded GC implementation as a global key" do | ||
| GC.config.should include(:implementation) | ||
| GC.config[:implementation].should be_kind_of(String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out, the string uses Encoding::US_ASCII, not UTF-8. Should this be specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't matter much, they are interchangeable (so I think not)
|
The docs at https://github.com/ruby/ruby/blob/2289961b485b1cbf7b1012693722c16a6cdb4cda/gc.rb#L398 say:
which sounds like maybe we shouldn't spec this, or explicitly allow the method to not be defined by Ruby implementations. OTOH, it's a public method on a public class, so it's not unlikely some gems might rely on it and indeed it seems better to just implement as you have shown in the description for increased compatibility. |
eregon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks goods, thanks!
From #1265
Includes generic tests and tests for MRI's default implementation of GC.
If a Ruby implementation does not want to expose implementation or configuration, a minimal method can be this:
This is the reference: https://github.com/ruby/ruby/blob/2289961b485b1cbf7b1012693722c16a6cdb4cda/gc.rb#L445