- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 984
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
add quiet logger #2161
base: master
Are you sure you want to change the base?
add quiet logger #2161
Conversation
@franciscomoloureiro could you please solve the merge conflict? |
/// </summary> | ||
[PublicAPI] | ||
[AttributeUsage(AttributeTargets.Class)] | ||
public class QuietModeAttribute : Attribute, IConfigSource |
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.
IMO there should be 1 attribute for all verbosity options and 1 cmd option instead of a separate attribute/option for each.
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.
I am not sure I understand this, the attribute/options are just different ways of configuring the same mode, with quiet=false you get the same output as before with quiet=true you only get the more relevant output
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.
Sorry, I'm mean about it: #190 (comment)
The PR should ideally implement all options, because it's not easy to add logging levels to the BDN.
It would be intuitive to see the following api:
[ConsoleVerbosity(Verbosity.Diagnostic, writeOutputFromGeneratedProject: true/false)]
[ConsoleVerbosity(Verbosity.Detailed)] // your PR does this level
[ConsoleVerbosity(Verbosity.Normal)]
[ConsoleVerbosity(Verbosity.Minimal)]
[ConsoleVerbosity(Verbosity.Quiet)]
cmd
dotnet run -c Release -- --verbosity diagnostic
dotnet run -c Release -- --verbosity detailed
dotnet run -c Release -- --verbosity normal
dotnet run -c Release -- --verbosity minimal
dotnet run -c Release -- --verbosity quiet
//short form:
dotnet run -c Release -- -v diag
dotnet run -c Release -- -v d
dotnet run -c Release -- -v n
dotnet run -c Release -- -v m
dotnet run -c Release -- -v q
If we implement your PR, it would be hard to do the standard .NET --verbosity
option.
And the naming [QuietMode]
is super confusing.
Below I have written 3 possible options. By my opinion it should mean 1) or 3).
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.
I understand now the full proposal, I had not seen last Adam's message on the issue, I will work on this during the next couple of days!
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.
It seems to me that this can be quite complicated.
My proposal is not final.
It's worth to post verbosity output to #190 (as mine) to start a discussion (because it affects the BDN a lot)
PS. If you don't do it, I'll get to it in a month. Any progress that you make will be very helpful!!
What the PR implement?
|
It is impementing 2. Running the benchmark with less output |
closes #190