kmsquire / Logging.jl

Logging package for julia
Other
43 stars 26 forks source link

RFC: Unexport logging functions #48

Open kmsquire opened 8 years ago

kmsquire commented 8 years ago

Looking for comments. My plan would be to release this as v0.5 or later.

Cc: @sbchisholm @aviks @DanielArndt @joshday @lostanlen @JoshChristie @fiatflux @zxteloiv

kmsquire commented 8 years ago

Thanks @JoshChristie. I think I found all of the needed renames.

Regarding keeping Logging.configure() vs solely relying on Logging.@configure(), that depends on how fast we want to break things.

Until now, the recommended way to use Logging was to call using Logging, and then use the logging functions directly. I would guess that most code does this, and and I would prefer a way to warn users with a deprecation, rather than break their code.

The way I do that here is by importing deprecated versions of the functions in the Logging.@configure macro call, with instructions on how to fix the offending code. This isn't possible from a function, and without that import, all code which calls the logging functions directly would fail.

Importing the functions in this way is a little sketchy--e.g., it doesn't work if you call Logging.@configure from a function, although it does fail gracefully, with a warning.

An alternative would be to issue the deprecation warning, and then follow through in a month or three (or, e.g., when Julia v0.6 comes out). That might be fine (and the resulting code would be simpler), although we would have to live with the current behavior during that time (overwriting Base.info and Base.warn).

I'll try to get that PR out for comparison, and then we can make a decision.

JoshChristie commented 8 years ago

Thanks for the explanation. That makes sense.

However, when i tried this out i get an error. Should this be a warning instead?

julia> using Logging
julia> Logging.configure(level=INFO)
ERROR: The functional form of Logging.configure(...) is no longer supported.
Instead, call
    Logging.@configure(...)
 in (::Logging.#kw##configure)(::Array{Any,1}, ::Logging.#configure) at ./<missing>:0

One last thing, renaming in the README.md should be Logging.@configure().

kmsquire commented 8 years ago

Yes, making it a warning would be better--I'll do that.

lostanlen commented 7 years ago

What is the status of this PR?