Skip to content
This repository was archived by the owner on Jan 2, 2023. It is now read-only.

Conversation

dmlambea
Copy link
Contributor

@dmlambea dmlambea commented Mar 3, 2017

Verbosity levels 0 (none), 1 (info) and 2 (extra) are supported. Current option 'quiet' is kept for backwards compatibility. It has been mapped to the new 'verbosity' values 0 (--quiet=true) and 1 (--quiet=false).

dmlambea added 2 commits March 3, 2017 09:54
…levels of logging. Currently, levels 0 (none), 1 (info) and 2 (extra) are supported.

Current 'quiet' option has been kept for backwards compatibility and it has been mapped to the new 'verbosity' values 0 (--quiet=true) and 1 (--quiet=false).
@ashkulz
Copy link
Member

ashkulz commented Mar 3, 2017

Looks good to me, but I think the description is incorrect. It should be

0 - no messages
1 - errors only
2 - warnings and errors

Also, making the default verbosity to 1 is a behavioral change, as it was previously 2. I think it'd be better to use an enum Verbosity with 3 values: Silent, Errors, WarningsAndErrors. Then, introduce a new command-line flag (--silent) which sets the verbosity to silent. Then, using both flags won't be an error: the last value wins. Your approach for backward compatibility can still be used, and is perfect -- you got it right 👍

Copy link
Member

@ashkulz ashkulz left a comment

Choose a reason for hiding this comment

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

see above.

@dmlambea
Copy link
Contributor Author

dmlambea commented Mar 3, 2017

I'd turn verbosity INTs into level names (--loglevel), if you think it's more appropriate: e.g., 0 would become 'silent', 1 -> 'error', 2 -> 'warn', 3 -> 'info' (which are ready to add more levels, like 'debug', if needed someday). As cumulative levels, 'warn' would include 'error' as well as 'info' would include 'warn' and 'error'. To keep behavior unchanged, the default '--loglevel' would be 'info' and the effect of using '--quiet' would be to set '--loglevel' to 'silent' (or 'none', as it's shorter, if it is Ok to you). The bindings would be enhanced with a new callback for leveled logging while keeping backwards compatibility.

I don't see the command-line flag '--silent' in the final picture, since both '--quiet' and '--loglevel silent' lead to the same behavior. If using '--quiet' and '--loglevel' other than 'silent': the last value wins.

Is this all Ok?

@ashkulz
Copy link
Member

ashkulz commented Mar 3, 2017

I'd prefer --log-level as the command-line parameter, but otherwise perfect 👍

@ashkulz ashkulz merged commit c754e38 into wkhtmltopdf:master Mar 7, 2017
@ashkulz
Copy link
Member

ashkulz commented Mar 7, 2017

Thanks for the contribution!

@ashkulz ashkulz added the Merged label Mar 7, 2017
@ashkulz ashkulz added this to the 0.12.5 milestone Mar 7, 2017
@dmlambea
Copy link
Contributor Author

dmlambea commented Mar 7, 2017

Thanks to you! Keep up the good work!

@ashkulz
Copy link
Member

ashkulz commented May 30, 2018

A release candidate is available which includes the fixes made in this PR -- please test and report your findings before the final release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

2 participants