Replace logback with scribe #3
Conversation
| } | ||
|
|
||
| object Logger { | ||
| object StdoutLogger extends Logger { |
There was a problem hiding this comment.
I tried to keep bells and whistles to an absolute minimum while still having at least one out-of-the-box working logger that doesn't suck.
| @@ -0,0 +1,17 @@ | |||
| package scala.meta.jsonrpc | |||
|
|
|||
| class SourceLocation(val file: String, val line: Int) { | |||
There was a problem hiding this comment.
Source location for error messages is too helpful for debugging that I couldn't resist to introduce this.
There was a problem hiding this comment.
This is problematic for us because when we extend it we need to implement it, and our loggers don't use source locations. Would it be possible you introduce implicit locations in an internal logger that extends the logger interface you'd expose to implementors?
|
Review @jvican |
| @@ -0,0 +1,17 @@ | |||
| package scala.meta.jsonrpc | |||
|
|
|||
| class SourceLocation(val file: String, val line: Int) { | |||
There was a problem hiding this comment.
This is problematic for us because when we extend it we need to implement it, and our loggers don't use source locations. Would it be possible you introduce implicit locations in an internal logger that extends the logger interface you'd expose to implementors?
|
You can discard the location if you don't need it. The implicit needs to be
in the interface to drive code generation at call site, it can't be
implemented with subtype of logger.
…On Fri, 27 Apr 2018 at 08:20, Jorge ***@***.***> wrote:
***@***.**** commented on this pull request.
I'm mostly concerned about SourceLocation being an implicit in the top
logger interface of lsp4s. Otherwise LGTM!
------------------------------
In jsonrpc/src/main/scala/scala/meta/jsonrpc/SourceLocation.scala
<#3 (comment)>:
> @@ -0,0 +1,17 @@
+package scala.meta.jsonrpc
+
+class SourceLocation(val file: String, val line: Int) {
This is problematic for us because when we extend it we need to implement
it, and our loggers don't use source locations. Would it be possible you
introduce implicit locations in an internal logger that extends the logger
interface you'd expose to implementors?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABV8XZWiXZHbcGF7Io9i__7_IAmIfs_bks5tstTIgaJpZM4To6Qt>
.
|
Given that lsp4s will be an external dependency, it won't be easy to possible to add a new logging statement to quickly debug what's happened.
|
I removed SourceLocation. |
It's ~200kb, usable out of the box, includes source information and supports Scala.js (which might be useful to implement lsp4s clients)
There was a problem hiding this comment.
OK, I ended up using https://github.com/outr/scribe to avoid reinventing the wheel on logging.
|
Nice, I admit I was getting concerned about accidentally re-inventing a new logging library 😅 |
|
I had forgotten about scribe, I eliminated slf4j since I want to keep the ability to cross-build for Scala.js open (might be handy for editor clients). |
I was going to ask about it! Cool 👍 |
olafurpg
changed the title
No description provided.