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

Removed logback.xml hack without using test/resources #287

Merged

Conversation

anthony-khong
Copy link
Member

No description provided.

@@ -16,7 +16,8 @@
[lein-cloverage "1.2.1"]
[lein-midje "3.2.1"]
[midje "1.9.9"]
[techascent/tech.ml.dataset "5.00-alpha-22"]
[techascent/tech.ml.dataset "5.00-alpha-22"
:exclusions [ch.qos.logback/logback-classic]]
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI, @behrica. I asked Chris how to get this done :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably the "best" would be to have any logging implementation depedency removed from tech.ml.dataset itself.
I am not 100% for Clojure,

but I still believe that a "library" (such as TMD) should never bring in any concrete logger as dependency,
but log against "interfaces."

Only an "application" should have concrete loggers as dependency.

A concrete logger could be added in "repl profile" only, as a repl is a kind of "application".

Choose a reason for hiding this comment

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

This is an area where I am certainly not certain but my position here has been somewhat carefully considered.

Here are my assumptions:

  1. You cannot use the system safely without logging. There are warnings from the C subsystems that for instance indicate that your program is going to blink out of existence leaving you wondering if perhaps programming was a bad career choice ;-).
  2. If you remove the logging system then the simplest possible project that could use this system correctly (meaning getting log messages at all) is somewhat more complex.
  3. The complex case of using the library is slightly simpler and more orthogonal.

I expect more mature projects, ones that have their own logging system setup, will have reasonable and predictable ways to override the tmd defaults. I put bias in towards simpler projects as those will more often be 'first time' projects of people who may not want to get involved whatsoever in the complete shitshow that is Java logging.

To this end upstream I use clojure.tools.logging which has zero extra dependencies but the first time I bring in a project that requires logging as part of it's setup (this is Smile) I force a safe default that can be overridden downstream.

Here is one command line use case from an issue posted earlier:

clj -Sdeps '{:deps {techascent/tech.ml.dataset {:mvn/version "5.00-alpha-21"}}}' -e "(require '[tech.v3.dataset :as ds])"

I am very curious as to everyone's thoughts on this, however, so please chime in if this strikes you some way.

@codecov-io
Copy link

Codecov Report

Merging #287 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           develop      #287   +/-   ##
=========================================
  Coverage   99.778%   99.778%           
=========================================
  Files           37        37           
  Lines         3162      3162           
  Branches         7         7           
=========================================
  Hits          3155      3155           
  Partials         7         7           

@anthony-khong anthony-khong merged commit 32e8429 into zero-one-group:develop Nov 6, 2020
@anthony-khong anthony-khong deleted the remove-logback-xml-hack branch January 29, 2021 07:19
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.

None yet

4 participants