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

Improve Logger type signatures related to level. #1046

Merged
merged 1 commit into from
Jul 21, 2022

Conversation

myronmarston
Copy link
Contributor

As shown in the current docs, level can be a String, Symbol, or Integer:

https://github.com/ruby/ruby/blob/b180ffa62270dc32bb69167822b4c698def5c8f7/lib/logger.rb#L388-L393

@ksss
Copy link
Collaborator

ksss commented Jun 27, 2022

@myronmarston
Copy link
Contributor Author

myronmarston commented Jun 27, 2022

How about using _ToS interface?

https://github.com/ruby/ruby/blob/232e2f598103c8eda37d08913665b72b6f787e3f/lib/logger.rb#L401

If I used that, then I believe code like this would type check:

logger = Logger.new("some/file.log", level: Object.new)
logger.level = Object.new

In fact, any object that implements to_s (which everything does except things that inherit directly from BasicObject, IIRC) would type check, but runtime errors will usually result.

An exception is thrown if the string returned by to_s, when downcased, doesn't exactly match one of debug, info, warn, error, fatal or unknown. As a general rule, the only types on which to_s returns a string that will match one of those are String and Symbol. Of course, someone could override to_s on their custom type to have it return one of those values (or someone can monkey patch a built-in type to have it return one of those values...) but in practice I've never seen anyone do that.

In short, while the line you linked to does use .to_s, I believe it does that because it's the "least effort" way to make the method support strings or symbols, not because it intends (or expects) to support any arbitrary object that implements to_s.

@ksss
Copy link
Collaborator

ksss commented Jun 28, 2022

@myronmarston You are right.

The _ToS interface is too broad and the current definition is too narrow.
There is a way to specify literals, but it is downcase and there are too many combinations.
The patch way would be a good balance.


I found the original discussion.

ruby/ruby#1101
https://bugs.ruby-lang.org/issues/11695

support symbol and string log level setting

It is clear from this that only String and Symbol are intended.

@soutaro soutaro merged commit 9e6c3c8 into ruby:master Jul 21, 2022
@myronmarston myronmarston deleted the myron/fix-logger-initialize branch July 21, 2022 00:27
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.

3 participants