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: Unexport logging functions #48

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

kmsquire
Copy link
Owner

@kmsquire kmsquire commented Oct 10, 2016

  • Two of the logging functions (info and warn) conflict with
    functions in base, and error was originally named err for the same
    reason. Unexporting them removes these conflicts.

  • With this change, the functions will (eventually) have to be qualified.

    using Logging
    Logging.@configure(level=DEBUG)
    
    Logging.debug("Debug message")
    Logging.info("Information for you!")
    Logging.warn("This is a warning.")
    Logging.error("You did something wrong!")
    Logging.critical("Danger Will Robinson!")

    A user can still call these directly without qualification (and without override warnings) with

    using Logging
    import Logging: debug, info, warn, error, critical
    Logging.@configure(level=DEBUG)
    
    debug("Debug message")
    info("Information for you!")
    warn("This is a warning.")
    error("You did something wrong!")
    critical("Danger Will Robinson!")
  • Also deprecate Logging.err in favor of Logging.error. This wasn't possible before because of conflicts with Base.error

  • In order to properly deprecate the non-qualified version, the Logging.configure function was also removed in favor of Logging.@configure, and the unqualified names are imported (with a deprecation warning) when the user calls Logging.@configure, but only if 1) Logging was included with using Logging, and 2) the user hasn't already imported the names manually. (This required a few shenanigans, and might be brittle, but should work most of the time and should only be temporary--e.g., until Julia v0.6).

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

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

Copy link

@JoshChristie JoshChristie left a comment

Choose a reason for hiding this comment

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

This is really great.

Is there a way that we can keep Logging.configure()? Having a macro in the middle could be annoying.

README.md Outdated
Logging.debug("debug message")
Logging.info("info message")
Logging.warn("warning message")
Logging.err("error message")
Copy link

@JoshChristie JoshChristie Oct 10, 2016

Choose a reason for hiding this comment

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

Should this be Logging.error("") now? (Saw a few more of these around)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks, fixed now!

@kmsquire
Copy link
Owner Author

kmsquire commented Oct 11, 2016

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
Copy link

JoshChristie commented Oct 11, 2016

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
Copy link
Owner Author

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

@lostanlen
Copy link

What is the status of this PR?

@kmsquire kmsquire force-pushed the kms/unexport_functions branch 2 times, most recently from c3a1c2b to 96678bc Compare May 28, 2017 07:32
kmsquire and others added 9 commits June 2, 2017 00:49
* Rename remaining calls to `err` -> `error`
* Fix test function for whether macros are loaded
* Internally, call `_configure` instead of `configure`
* Fix `Symbol` depwarn in test
* Use `Compat` for v0.4 compatibilty
* fix keyword handling in julia v0.6
* use Compat: @static
* macro hygiene fixes
* ensure that macros work inside modules too
@kmsquire kmsquire force-pushed the kms/unexport_functions branch from 96678bc to 3da6d9e Compare June 2, 2017 07:59
* Not needed for functionality, and allows us not to depend on Compat.jl
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

Successfully merging this pull request may close these issues.

6 participants