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

[RFC] Stopping use of depwarn intended to inform to "end users" #419

Closed
kimikage opened this issue Apr 23, 2020 · 6 comments
Closed

[RFC] Stopping use of depwarn intended to inform to "end users" #419

kimikage opened this issue Apr 23, 2020 · 6 comments

Comments

@kimikage
Copy link
Collaborator

kimikage commented Apr 23, 2020

In Julia v1.5 series (or nightly build),

  • Deprecation warnings are no longer shown by default. i.e. if the --depwarn=... flag is not passed it defaults to --depwarn=no.

cf. JuliaLang/julia#35362

Therefore, depwarns will be for package developers, not for end users. (We use depwarn instead of normal warn just because it's annoying.:sweat:)

There are 4 depwarns used in this package.

Base.hex

Colors.jl/src/utilities.jl

Lines 103 to 106 in a447509

function Base.hex(c::Colorant)
Base.depwarn("Base.hex(c) has been moved to the package Colors.jl, i.e. Colors.hex(c).", :hex)
hex(c)
end

Perhaps the end users won't explicitly specify Base, so we can leave this. (We can also remove this.)

hex

Colors.jl/src/utilities.jl

Lines 109 to 116 in a447509

function hex(c::ColorAlpha)
Base.depwarn("""
The alpha position for $(typeof(c)) (<:ColorAlpha) will soon be changed.
You can get the alpha-first style string by `hex(c, :AARRGGBB)` or `hex(c |> ARGB32)`.
""", :hex)
#_hex(HexNotation{RGBA,:upper,8}, c) # breaking change in v1.0
_hex( HexNotation{ARGB,:upper,8}, c) # backward compatible
end

This is a problem as the new behavior will not involve any API changes. Also, hex is a function used directly by the end users. However, I don't think we need to show this depwarn to the end user when Julia v1.5 is released. So, we can leave this.

ColorTypes.color

Colors.jl/src/parse.jl

Lines 225 to 228 in f1047da

@noinline function ColorTypes.color(str::AbstractString)
Base.depwarn("color(\"$str\") is deprecated, use colorant\"$str\" or parse(Colorant, \"$str\")", :color)
parse(Colorant, str)
end

This is a proper use case of depwarn, but I don't think it's necessary anymore. Moreover, color is defined in ColorTypes and exported (and re-exported).

parse

Colors.jl/src/parse.jl

Lines 143 to 147 in f1047da

Base.depwarn(
"""
The X11 color names with spaces are not recommended because they are not allowed in the SVG/CSS.
Use "$camel" or "$wo_spaces" instead.
""", :parse)

This has been a problem since the beginning. (cf. #390)
This is mainly for end users, so it doesn't make sense if it disappears. At least this is what should be displayed as a normal warn.

@kimikage
Copy link
Collaborator Author

kimikage commented Apr 23, 2020

This is off-topic but I don't understand CI failures on nightly build.

Parse: Log Test Failed at /home/travis/build/JuliaGraphics/Colors.jl/test/parse.jl:17
  Expression: parse(Colorant, "sea GREEN")
  Log Pattern: (:warn, r"Use \"SeaGreen\" or \"seagreen\"")
  Captured Logs: 

Pkg.test uses --depwarn=yes now (cf. JuliaLang/Pkg.jl#1763).

Edit:
cf. JuliaLang/julia#35584

@johnnychen94
Copy link
Member

johnnychen94 commented Apr 23, 2020

This is becoming too nitpicking and seems like an antipattern wrt JuliaLang/julia#35362

End-users don't read warnings. They don't even read error messages. Since the old API works fine, he could choose to not update his API accordingly, why must he see these warnings even if he set Compat correctly wrt semver?

The new practice after JuliaLang/julia#35362, IIUC, is to switch on the depwarn option when you try to add a new breaking version in compat and then fix all depwarns in one PR.

@kimikage
Copy link
Collaborator Author

End-users don't read warnings. They don't even read error messages.

Indeed. Similarly, package developers do not read the commit logs or release notes. 😛

@kimikage
Copy link
Collaborator Author

I'm thinking of removing the depwarn in parse. For the time being, I do not intend to ban the names with spaces. The document has already stated that they are not recommended. (Some users do not read the document, though. 😛)

@kimikage
Copy link
Collaborator Author

Gotcha! I think we were not on the same wavelength because of my poor English, @johnnychen94.

@kimikage kimikage changed the title [RFC] Stopping use of depwarn to inform to "end users" [RFC] Stopping use of depwarn intended to inform to "end users" Apr 24, 2020
@kimikage
Copy link
Collaborator Author

kimikage commented Jun 7, 2020

Let's discuss the depwarns in separate PRs.

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

No branches or pull requests

2 participants