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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants