Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Adding in --quiet and --log-level flags #694

Merged
merged 6 commits into from
Jan 22, 2020
Merged

Adding in --quiet and --log-level flags #694

merged 6 commits into from
Jan 22, 2020

Conversation

Pauan
Copy link
Contributor

@Pauan Pauan commented Jul 24, 2019

No description provided.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Code looks fine to me. I wonder however if --quiet should mean that we don't emit any output at all and just use the exit code. This is what many standard tools, such as diff, do when given -q. I think cargo build --quiet does this as well.

@Pauan
Copy link
Contributor Author

Pauan commented Jul 25, 2019

I wonder however if --quiet should mean that we don't emit any output at all and just use the exit code.

I did consider that, however I do actually want the cargo build output to show, since building a crate can take a long time and the user wants to see progress being made.

(It's possible to achieve full silence by using wasm-pack --quiet build -- --quiet though that's very non-obvious)

So maybe this should actually be a different flag, something like --message-format none (analogous to cargo --message-format)?

Or better yet, --log-level error, which would be even more customizable (you could also set --log-level info, --log-level debug, or --log-level warn).

@Pauan
Copy link
Contributor Author

Pauan commented Jul 25, 2019

Okay, I changed --quiet so it also silences output from cargo.

And I added in a new --log-level flag which controls wasm-pack's log level. So the previous behavior of --quiet can be achieved by using --log-level error.

Copy link
Member

@ashleygwilliams ashleygwilliams left a comment

Choose a reason for hiding this comment

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

i think this looks good! can we add a test and some docs?

@ashleygwilliams ashleygwilliams added needs docs please add docs to this PR needs tests please add tests to this PR and removed needs review labels Jul 25, 2019
@Pauan
Copy link
Contributor Author

Pauan commented Jul 26, 2019

can we add a test and some docs?

I don't mind, but the existing CLI flags (e.g. verbose) don't have that. So where would I put it?

@Pauan Pauan changed the title Adding in --quiet flag to suppress warnings Adding in --quiet and --log-level flags Jul 26, 2019
@Pauan
Copy link
Contributor Author

Pauan commented Jul 26, 2019

(Just a heads up that I added in -q as a short-hand for --quiet, which is consistent with cargo)

@ashleygwilliams
Copy link
Member

@Pauan i'd probably create a new doc under commands about log level! https://rustwasm.github.io/wasm-pack/book/commands/index.html

@Pauan
Copy link
Contributor Author

Pauan commented Jul 26, 2019

@ashleygwilliams I added in docs and unit tests.

@Pauan
Copy link
Contributor Author

Pauan commented Jul 31, 2019

@ashleygwilliams Ping.

1 similar comment
@Pauan
Copy link
Contributor Author

Pauan commented Aug 24, 2019

@ashleygwilliams Ping.

@niedzielski
Copy link

This is a subtle but important change as verbose informational output is useful for debugging but may conceal important issues. (Silence is golden!)

@Pauan
Copy link
Contributor Author

Pauan commented Jan 9, 2020

@ashleygwilliams This is ready for review.

@Pauan Pauan removed needs docs please add docs to this PR needs tests please add tests to this PR labels Jan 22, 2020
@Pauan Pauan merged commit 846b989 into rustwasm:master Jan 22, 2020
@Pauan Pauan deleted the quiet branch January 22, 2020 21:00
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants