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

Deprecate Optional<Logger> extensions #42

Merged
merged 1 commit into from
Sep 14, 2018
Merged

Conversation

ejensen
Copy link
Contributor

@ejensen ejensen commented Sep 7, 2018

Deprecates Optional<Logger> extensions.

This encourage the use of non-optional log vars. Which can be assigned to .disabled.
This mimics the OSLog API.

The migration is simple: var log: Logger?var log: Logger = .disabled.

Copy link
Contributor

@AtomicCat AtomicCat left a comment

Choose a reason for hiding this comment

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

Been thinking about this ever since WWDC. Thanks for making the change!

@cnoon cnoon self-requested a review September 11, 2018 18:55
@cnoon cnoon self-assigned this Sep 11, 2018
@cnoon cnoon added the logger label Sep 11, 2018
Copy link
Member

@cnoon cnoon left a comment

Choose a reason for hiding this comment

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

I like the change for sure, but we need to address the backwards compatibility.

@@ -50,6 +50,9 @@ open class Logger {

// MARK: - Properties

/// A logger that does not output any messages to writers.
public static let disabled: Logger = NoOpLogger()
Copy link
Member

Choose a reason for hiding this comment

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

This is an awesome change, but it's not backwards compatible. Are you thinking this should go out as Willow 6? If yes, then I'd recommend that we deprecate the optional APIs for now and add the disabled Logger. Then we commit this into a willow6 branch or something for when we're ready to lump more breaking changes into it. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are considering a major version bump, there are other things I'd like us to consider. The Writer API feels clunky and I would want to maybe take another look at how os_log and os_signpost work these days and see if there is anything for us to learn from. I also think the default ConsoleWriter could have cleaner output... it's the first thing I replace every time I use Willow...

final private class NoOpLogger: Logger {
init() {
super.init(logLevels: .off, writers: [])
enabled = false
}
Copy link
Member

Choose a reason for hiding this comment

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

Awesome!

@ejensen ejensen force-pushed the remove-optional-extensions branch from d64b8e1 to 23421b3 Compare September 11, 2018 22:44
Encourage using non-optional log variables that are can be assigned to .disabled.
This mimics the OSLog API
@ejensen ejensen force-pushed the remove-optional-extensions branch from 23421b3 to eca5c61 Compare September 11, 2018 22:52
@ejensen ejensen changed the title Remove Optional<Logger> extensions Deprecate Optional<Logger> extensions Sep 13, 2018
@cnoon cnoon merged commit ec7f3ee into master Sep 14, 2018
@cnoon cnoon deleted the remove-optional-extensions branch September 14, 2018 14:53
@cnoon cnoon added this to the 5.1.0 milestone Sep 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants