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

validate target argument #484

Merged
merged 1 commit into from
Jan 11, 2019
Merged

validate target argument #484

merged 1 commit into from
Jan 11, 2019

Conversation

csmoe
Copy link
Member

@csmoe csmoe commented Jan 10, 2019

Closes #483

@danwilhelm
Copy link
Member

This is a very helpful addition! A few quick things I noticed:

@csmoe
Copy link
Member Author

csmoe commented Jan 10, 2019

@danwilhelm checking for buildmode was already there:

impl FromStr for BuildMode {
type Err = Error;
fn from_str(s: &str) -> Result<Self, Error> {
match s {
"no-install" => Ok(BuildMode::Noinstall),
"normal" => Ok(BuildMode::Normal),
"force" => Ok(BuildMode::Force),
_ => bail!("Unknown build mode: {}", s),
}
}
}

@danwilhelm
Copy link
Member

Sorry I was unclear!

In my comment, the first link points to the mode parameter in TestOptions. I believe the default value and help string there should mirror the one changed in this PR (ie remove the default value "normal" and add "force" to the help string).

In the second link, as you point out, testing for Build Mode is already there. The Issue this PR addresses was that when an invalid argument is passed, the valid options are not displayed. Instead of bailing with 'Unknown build option', it could indicate that valid options are no-install, normal, and force.

danwilhelm added a commit to danwilhelm/wasm-pack that referenced this pull request Jan 11, 2019
Issue rustwasm#484.

PR392 inadvertantly replaced the `login` interactive process spawner
with
`child::run`, which is hard-coded to buffer stdout/stderr. This caused
`login` to become essentially unusable; the user could no longer see
interactive input prompts or error messages displayed by `npm adduser`.

The code was not directly reverted because the previous version:
1. Returned Error instead of failure::Error. (Updated to use
`bail!`, which is consistent with `publish`.)
2. Displayed all stderr only upon exit, rather than interactively
displaying it. This led to repeated interactive prompts without
informing the user why. (Updated to use `status()` which inherits
stdin/stdout/stderr by default.)
3. Did not provide logging. (Now duplicates the logging in
`child::run`.)
@ashleygwilliams
Copy link
Member

this seems to have a bunch of failures on appveyor-- a large number that seems disproportionate to the change here:

failures:
    bindgen::can_download_prebuilt_wasm_bindgen
    build::build_different_profiles
    build::it_format_out_dir_on_windows
    build::it_should_build_crates_in_a_workspace
    build::it_should_build_js_hello_world_example
    build::it_should_build_nested_project_with_transitive_dependencies
    build::renamed_crate_name_works
    test::cdylib_not_required
    test::complains_about_missing_wasm_bindgen_test_dependency
    test::it_can_find_a_webdriver_on_path
    test::it_can_run_browser_tests
    test::it_can_run_failing_tests
    test::it_can_run_node_tests
    test::it_can_run_tests_with_different_wbg_test_and_wbg_versions
    test::it_requires_node_or_a_browser
    test::renamed_crate_name_works
    test::the_headless_flag_requires_a_browser
test result: FAILED. 31 passed; 17 failed; 0 ignored; 0 measured; 0 filtered out

@csmoe do you have a windows computer to test this locally? if not perhaps @steveklabnik ? i would be surprised if this was all from this incredibly small change...

@csmoe
Copy link
Member Author

csmoe commented Jan 11, 2019

@ashleygwilliams I have only one linux machine but i'll recheck the failed test cases.

@ashleygwilliams
Copy link
Member

i'm gonna restart the build- perhaps it was a weird flake on appveyors part

@ashleygwilliams
Copy link
Member

looks like it was a fluke! i think this is good to go and we can merge.

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.

thank you so much!

@ashleygwilliams ashleygwilliams merged commit 4cd7790 into rustwasm:master Jan 11, 2019
ashleygwilliams pushed a commit that referenced this pull request Feb 27, 2019
Issue #484.

PR392 inadvertantly replaced the `login` interactive process spawner
with
`child::run`, which is hard-coded to buffer stdout/stderr. This caused
`login` to become essentially unusable; the user could no longer see
interactive input prompts or error messages displayed by `npm adduser`.

The code was not directly reverted because the previous version:
1. Returned Error instead of failure::Error. (Updated to use
`bail!`, which is consistent with `publish`.)
2. Displayed all stderr only upon exit, rather than interactively
displaying it. This led to repeated interactive prompts without
informing the user why. (Updated to use `status()` which inherits
stdin/stdout/stderr by default.)
3. Did not provide logging. (Now duplicates the logging in
`child::run`.)
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.

3 participants