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

Change to INFO when alerting users about missing fields Cargo.toml #394

Merged
merged 3 commits into from
Oct 5, 2018

Conversation

mstallmo
Copy link
Member

@mstallmo mstallmo commented Oct 5, 2018

Closes #393

I switched the level displayed in the progress bar from PBAR.warn to PBAR.info and changed the format closure from warn_format to info_format for consistency.

How the new terminal output looks:
info_output

Make sure these boxes are checked! 📦✅

  • You have the latest version of rustfmt installed and have your
    cloned directory set to nightly
$ rustup override set nightly
$ rustup component add rustfmt-preview --toolchain nightly
  • You ran rustfmt on the code base before submitting
  • You reference which issue is being closed in the PR text

✨✨ 😄 Thanks so much for contributing to wasm-pack! 😄 ✨✨

@ashleygwilliams
Copy link
Member

@fitzgen do you think this is enough to make it less scary, or should we also try to make it into a single info line?

(thanks @mstallmo this looks good to me but we may want to take it just a bit further- if you're up for it we can do it on this PR, otherwise i can accept this and do it today)

@mstallmo
Copy link
Member Author

mstallmo commented Oct 5, 2018

@ashleygwilliams No problem, I'm up for updating this PR to reflect whatever we think is the less scary way to inform users of the missing fields!

@ashleygwilliams
Copy link
Member

@mstallmo if you're up for it, would you be willing to implement this feature where the missing fields are a single INFO? i think that's probably the best case here

@mstallmo
Copy link
Member Author

mstallmo commented Oct 5, 2018

Will do! I can probably get to this in the next couple of hours over lunch.

For formatting I’m assuming we just want to have the missing fields separated by commas in the INFO line?

@ashleygwilliams
Copy link
Member

@mstallmo yeah, separated by commas sounds good to me, e.g. "Optional fields missing from Cargo.toml: 'x', 'x', 'x'. These are not necessary but recommended."

@fitzgen
Copy link
Member

fitzgen commented Oct 5, 2018

@fitzgen do you think this is enough to make it less scary, or should we also try to make it into a single info line?

I think switching both to INFO and to single line is good 👍

@fitzgen fitzgen removed their request for review October 5, 2018 18:16
@mstallmo
Copy link
Member Author

mstallmo commented Oct 5, 2018

I've updated to combine the INFO print out to one line with fields separated by commas. I've attached pictures how the print out looks with the various combinations of printout:

one

two

three

Side note: It looks like running rustfmt on the mod.rs file picked up a change with how .map was formatted on line 84. If we don't want to include that here I can reset the commit and undo that change.

@ashleygwilliams
Copy link
Member

@mstallmo appreciate the note on the diff but i think it's fine :) this all looks awesome- thanks for your efforts. waiting on CI to be green but i think we can merge this!

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.

🎉

@mstallmo
Copy link
Member Author

mstallmo commented Oct 5, 2018

Welp, just ran cargo fmt and it looks like the formatting for visual studio on my Mac was not configured correctly. I fixed the issue and updated.

@ashleygwilliams ashleygwilliams merged commit 921b724 into rustwasm:master Oct 5, 2018
@ashleygwilliams ashleygwilliams added this to the 0.5.1 milestone Oct 9, 2018
xmclark added a commit to xmclark/wasm-pack that referenced this pull request Oct 29, 2018
Change to INFO when alerting users about missing fields in Cargo.toml

Combine filed missing messages into one INFO line

Fix bad formating

Merge pull request rustwasm#394 from mstallmo/master

Change to INFO when alerting users about missing fields Cargo.toml
child: Always print everything to our output

Also don't buffer the child's stdout and stderr.

error: Add stdout to the `Error::Cli` variant

refactor: Return failure::Error instead of wasm_pack::error::Error

refactor: Import self and use full module path for failure

Use full module path for failure to be consistent since
it's used like that in other modules.

refactor: Return failure::Error instead of wasm_pack::error::Error

chore: Run rustfmt

chore: Run rustfmt

Merge pull request rustwasm#392 from fitzgen/child-process-and-output-management

Child process and output management
Merge pull request rustwasm#401 from drager/return-failure-error

Return `Result<T, failure::Error>` instead of `Result<T, wasm_pack::error::Error>`
v0.5.1

Merge pull request rustwasm#404 from rustwasm/0.5.1

v0.5.1
feat(website): bump vers

Merge pull request rustwasm#405 from rustwasm/website-update

feat(website): bump vers
test(command/build): add a test for build command

useing local wasm-bindgen

Fix typo in test function name for copying the README

bugfix(bindgen-target-dir): use PathBuf to join

the old code are hard coded path with "/", which may cause error
on windows, thus changing to use PathBuf.join.

fixing rustwasm#414

change for cargo_metadata

Merge branch 'master' into test-build-for-example
Merge pull request rustwasm#408 from huangjj27/test-build-for-example

test(command/build): add a test for build command
Merge pull request rustwasm#412 from mstallmo/typo-fix

Fix typo in test function name for copying the README
Merge branch 'master' into fix-canonical-paths-on-windows
this change is not related to this PR


use absolutize


remove unused use


cargo fmt
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.

Warnings about missing Cargo.toml keys are scarier than need be
3 participants