Skip to content
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

New commandline option 'verbosity' to allow different log levels #3364

Merged
merged 9 commits into from Mar 7, 2017

Conversation

Projects
None yet
2 participants
@dmlambea
Copy link
Contributor

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 some commits Mar 3, 2017

Added a new commandline option 'verbosity', to support more than two …
…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

This comment has been minimized.

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 👍

@ashkulz
Copy link
Member

ashkulz left a comment

see above.

@dmlambea

This comment has been minimized.

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

This comment has been minimized.

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ashkulz

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

dmlambea commented Mar 7, 2017

Thanks to you! Keep up the good work!

@ashkulz

This comment has been minimized.

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 join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.