-
Notifications
You must be signed in to change notification settings - Fork 63
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
Move logging from hslogger to co-log #347
Conversation
cc @eddiemundo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Only have super minor formatting tweaks. Seems like the tests don't compile though, and 8.6.5 needs an extension for the forall
. Thanks for all the work, haven't had time to do anything except follow HLS superficially...
Thank you for bringing co-log to hie-bios! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, looking forward to it!
If you have troubles with the tests (because they are hard-to-read), give me a shout and I can help out!
import Data.Version (showVersion) | ||
import Data.Text.Prettyprint.Doc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub Actions scream a bunch of deprecation warnings here. Can we do something about that? I remember, there was an issue with my PR haskell/haskell-language-server#2352
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not sure what the best approach is. Pepe really wants to keep compatibility with old prettyprinter versions. That gives us two options: 1) use the deprecated import, 2) use CPP. I hate 1 less but I'm happy to take the other bad option also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, for now, just leave it be. Eventually we can just drop it, probably.
This lines up with the way that logging is being done in `haskell-language-server` and `lsp`. At the moment `hie-bios` is the only reason HLS has a `hslogger` dependency, and it means that the logs from `hie-bios` are inconsistent with those from the rest of the subsystems.
Fixed the tests. Somehow I just didn't realise they existed 😅 I generally stopped them from issuing a step every log line, which seemed excessive, and instead just put in a couple of explicit steps. CI is green. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, awesome!
This lines up with the way that logging is being done in
haskell-language-server
andlsp
.At the moment
hie-bios
is the only reason HLS has ahslogger
dependency, and it means that the logs from
hie-bios
are inconsistentwith those from the rest of the subsystems.