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

Fix logs for uniformity (#716) #723

Merged
merged 3 commits into from
Dec 22, 2020
Merged

Conversation

petosorus
Copy link

No description provided.

Copy link
Member

@drager drager left a comment

Choose a reason for hiding this comment

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

Thanks @petosorus!

I tried this out and the output is not really unified. Now for warnings it looks like this: [WARN] :-): origin crate has no README (Notice the : after the smiley). The : should come after [WARN].

This also makes one test to break, since the test checks for e.g. [WARN]:

@petosorus
Copy link
Author

Yeah, I'm a bit dumb and made this change a bit too quickly. Thanks for the feedback.

@petosorus
Copy link
Author

Regarding the broken test, should I change "[WARN]: \"package.metadata.wasm-... to "[WARN]: :-) \"package.metadata.wasm-... as the reference string we test against, or do you think there's a way to ignore the :-) part as it's not that important, and maybe subject to change?

This is in manifest.rs, line 451. I don't know if predicates::str::contains would allow something like a regex similar to this "[WARN]: (:-\))? \"package.metadata.wasm-

@drager
Copy link
Member

drager commented Oct 3, 2019

Regarding the broken test, should I change "[WARN]: \"package.metadata.wasm-... to "[WARN]: :-) \"package.metadata.wasm-... as the reference string we test against, or do you think there's a way to ignore the :-) part as it's not that important, and maybe subject to change?

This is in manifest.rs, line 451. I don't know if predicates::str::contains would allow something like a regex similar to this "[WARN]: (:-\))? \"package.metadata.wasm-

Yeah, I think you can check for [WARN]: :)

Copy link
Member

@drager drager left a comment

Choose a reason for hiding this comment

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

@petosorus Sorry for my late response! This now works good. Would be great if you could rebase master into your branch and then we can get this merged 😊

@drager drager added this to the 0.10.0 milestone Feb 13, 2020
@petosorus
Copy link
Author

Hi! I'm still not too sure about rebase, should have done what's needed :)

@drager
Copy link
Member

drager commented Feb 18, 2020

@petosorus Thanks! It looks good! There's something weird going on with the checks. Once those passes we can get this merged 😊

@ashleygwilliams ashleygwilliams merged commit d46d1c6 into rustwasm:master Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants