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

Remove symbol overloads of Colorize color methods for symbol auto-casting #11775

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

straight-shoota
Copy link
Member

Colorize methods receiving a color (#fore and #back) have overloads receiving a Symbol parameter which is manually mapped to values of ColorANSI. This implementation predates the symbol auto-casting of enum values.

This patch removes the overloads with Symbol typed parameters which leaves the overloads receiving a Color parameter. This type is a union including the enum ColorANSI which enables symbol auto-casting for color values.

The problem is: it's a breaking change. If the API is used with symbols that are not directly created from literals, there will be a compile time error. This is good and intended because it adds type safety (you can know at compile time whether a value is valid). But it's still a breaking change that destroys valid use cases which would require changes to user code (as demonstrated by the changes in samples/2048.cr and src/spec/dsl.cr for example). So I believe we can't merge it just like that.
The only option is to leave the removed overloads in with a deprecation notice. That's not ideal though because the deprecation warning even triggers for usages that are compatible with the auto-casting method. But the symbol overload takes precedence over auto-casting. The only way to deactivate deprecation warnings is using a named argument: fore(fore: :green).

Are there any other ideas? We could use a feature flag but I think would be silly for such a minor thing.

This patch has been extracted from #7690.

@HertzDevil
Copy link
Contributor

This is observed in #11020 and also in:

# Returns the `#offset` formatted as `+HH:mm:ss`.
# When *with_colon* is `false`, the format is `+HHmmss`.
#
# When *with_seconds* is `false`, seconds are omitted; when `:auto`, seconds
# are omitted if `0`.
def format(with_colon = true, with_seconds = :auto)
String.build do |io|
format(io, with_colon: with_colon, with_seconds: with_seconds)
end
end

I don't know any way other than as part of some other flag disallowing Symbol-typed variables altogether.

@@ -350,7 +307,7 @@ struct Colorize::Object(T)
self
end

def on(color : Symbol)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should there be a deprecation warning for this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the OP, the mere existence of a Symbol-accepting overload will trigger deprecations even for arguments that autocast to Color due to overload ordering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants