Log using Microsoft.Extensions.Logging.Abstractions; implement WpfProgram #255
Conversation
This comment has been minimized.
This comment has been minimized.
What do you think about extracting those strings to reduce the magic to something like this? Then auto-complete would help the user more quickly type these from scratch and get them correct.
|
Yes, let's go with 1 👍
For this use case I have never seen anyone defining constants (doesn't mean no-one is doing it, though). But it can't hurt. IMHO we should primarily document the normal string version and also mention that the strings are accessible in the |
|
Oh wait, I'm not sure it's a good idea with log category constants anyway. The categories are hierarchical, so one can use I think perhaps it's best if we just document the logger categories and have users use strings, as they probably are using everywhere else, too. |
|
For your review: I have not spent that much effort on log levels or logger categories. I think it can work as it is now, but I'm certainly open to other suggestions. For example, we may consider renaming |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I just closely reviewed all the file changes. I left some minor change suggestions. Otherwise, everything else looks good. |
Co-authored-by: Tyson Williams <34664007+bender2k14@users.noreply.github.com>
…o logging-microsoft
|
Everything ready, then? By the way, if you ever want to squash all commits in a PR before merging, you can do it using the dropdown on the Merge button. |
|
Yep, ready. I am such a perfectionist. Sometimes I want more than just one commit, and I am curious to see what the commit message is when you do it now. |
I have noticed 😆 You can choose the message yourself when doing it this way. By default it seems to set the first commit message line to the PR title, and it also includes every commit message on the subsequent lines (I removed those). |
Fixes #105
Logging implemented using
Microsoft.Extensions.Logging.Abstractions.I also had to create a separate Program type for Elmish.WPF. I think that has been mentioned/discussed previously, can't remember where.
Log levels can be discussed, but I think they are uncontroversial - most of what we log fits with
Tracesemantics (the most verbose level of logging), and other than that there are just a fewWarnings.Client usage is very simple and I'd say well-known in .NET. The samples are now using Serilog.
At a minimum, if users want to see logs from Elmish.WPF using Serilog, they have to install
Serilog.Extensions.Logging(and a chosen sink, e.g.Serilog.Sinks.Console). The Serilog logger is initialized as normal (e.g. the static logger):Then users simply use
SerilogLoggerFactoryin the call toWpfProgram.withLogger:Currently we log to four categories which the user can control. To see everything (as in the samples), users must do something similar to this:
(If they really just want it all, they can use
.MinimumLevel.Verbose()instead of the four lines above, but I chose to use those four lines in the samples to clearly show how to opt into specific kinds of logs.)I really like this way of logging and find it a good fit for Elmish.WPF. IMHO the dependency on
Microsoft.Extensions.Logging.Abstractionsis entirely unproblematic. The users have control over logging without having to write a lot of custom boilerplate.Heck, even the "lazy" behavior of binding measure logs is preserved this way. If
Trace(Verbosein Serilog) is not enabled for theElmish.WPF.BindingPerformancecategory, then no measurements are performed.We can of course adjust the categories if we want.