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.

Sign up for GitHub

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

Jump to bottom

Replace slog with log #434

Closed
wants to merge 4 commits into from
Closed

Replace slog with log #434

wants to merge 4 commits into from

Conversation

alexcrichton
Copy link
Contributor

This commit replaces the slog family of crates used by wasm-pack
with the log crate plus env_logger. This also means that by default
wasm-pack also won't create a wasm-pack.log file in the current
directory. Enabling logging will now be done through
RUST_LOG=wasm_pack instead of -v flags.

Closes #425

fitzgen
fitzgen approved these changes Nov 5, 2018
View reviewed changes
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.

LGTM! I think @ashleygwilliams should chime in on the whole concept before we merge this, though.

@fitzgen fitzgen added this to the 0.6.0 milestone Nov 6, 2018
@huangjj27 huangjj27 mentioned this pull request Nov 10, 2018
Merged
3 tasks
@otavio
Copy link

otavio commented Dec 5, 2018

I am looking for a logging solution and were looking at this repository as an example of the use of slog.

Even though I noticed you are not using structured logging, I wonder if it doesn't make sense to look at it. Considering wasm-pack library, the use of slog opens some nice opportunities to use custom Drain if embedding it inside another tool or service.

I may be completely mistaken here but would like to know your opinion about it. Mind to share your view on this?

@huangjj27
Copy link
Member

Hi @otavio, I think wasm-pack is used for development scene rather than embedded production. So, I think it’s ok not to use logger with embed feature.

@drager drager mentioned this pull request Dec 28, 2018
Closed
@alexcrichton
Replace slog with log
32d6482 
This commit replaces the `slog` family of crates used by `wasm-pack`
with the `log` crate plus `env_logger`. This also means that by default
`wasm-pack` also won't create a `wasm-pack.log` file in the current
directory. Enabling logging will now be done through
`RUST_LOG=wasm_pack` instead of `-v` flags.

Closes rustwasm#425
@alexcrichton
Copy link
Contributor Author

I've rebased this on master, should be good to go now

ashleygwilliams
ashleygwilliams approved these changes Jan 11, 2019
View reviewed changes
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.

thanks for all this hard work!

@huangjj27
Copy link
Member

huangjj27 commented Jan 12, 2019 •
edited
Loading

AppVeyor fails on a test base on wasm-pack.log which now is not generated by default. Add some params to the test to generate the log. see the test it_format_out_dir_on_windows in PR #396

@ashleygwilliams
Copy link
Member

closing in favor of #491

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fitzgen fitzgen fitzgen approved these changes

@ashleygwilliams ashleygwilliams ashleygwilliams approved these changes

Assignees
No one assigned
Labels
changelog - fix changelog - maintenance
Projects
None yet
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.
5 participants
@alexcrichton @otavio @huangjj27 @ashleygwilliams @fitzgen