search

gmethvin / directory-watcher

A cross-platform Java recursive directory watcher, with a JNA macOS watcher and Scala better-files integration
Apache License 2.0
265 stars 34 forks source link

Expose the logger in the watcher constructor #12

Closed jvican closed 6 years ago

jvican commented 6 years ago

I'd like to propose a change to the public API which makes the library more fitting for applications that do not use global mutable loggers.

In my case, I use directory-watcher in Bloop. Our application uses Nailgun under the hood, and its architecture forces us to avoid global mutable loggers (i.e. neither logback nor log4j), and have a strict control over stdout and stderr. As a result, Bloop has its own slf4j-compliant logger which are immutable and local, and so we create them per client.

This architecture is a little bit at odds with the current implementation of the library, which has a static, global logger in DirectoryWatcher. My suggestion is that we remove it and make DirectoryWatcher.create take the logger as a parameter, so that users are in control of the situation.

I took myself the liberty of implementing this at https://github.com/jvican/directory-watcher/commit/a1c0e21c92032863480b16a696eb59ce4b29b822, and I released it under my own organization so that I can directly depend on it (0.5.2-a1c0e21c) -- so there's no rush to get this merged on my side.

I open this ticket to raise some discussion. If we agree on this change, I'll open up a pull request. I believe my prototype can be massaged to avoid breaking changes downstream by duplicating all the create constructors to take a logger parameter, and those which don't (the current interface) would directly fallback on the previous logger implementation via Logger.getLogger.

gmethvin commented 6 years ago

I think the change you're suggesting is fine as long as we have overloaded versions that don't require the logger parameter. If logger is not passed it can default to Logger.getLogger(getClass()). This way the change is fully backwards compatible, and we keep the more convenient APIs for users who don't care about the logger.

I'm happy to accept a pull request.

jvican commented 6 years ago

I think the change you're suggesting is fine as long as we have overloaded versions that don't require the logger parameter. If logger is not passed it can default to Logger.getLogger(getClass()). This way the change is fully backwards compatible, and we keep the more convenient APIs for users who don't care about the logger.

Great, that's exactly what I describe in my last paragraph. I'll try to make a pull request with this change soon.

gmethvin commented 6 years ago

@jvican Just checking if you're still planning to work on this.

jvican commented 6 years ago

Cannot find the time now, pretty busy preparing stuff. If no one beats me to it and does it before Scaladays, I'll implement it after the conference.

olafurpg commented 6 years ago

I'd love to have the the ability to provide a custom Logger. I'm using this project in https://github.com/olafurpg/vork and currently library API users need to copy-paste the same logback.xml file into their project in order to suppress logback noise.

gmethvin commented 6 years ago

@olafurpg @jvican Working on this in #18.