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 using --help, --verbose, etc. #3620

Merged
merged 1 commit into from
Jul 14, 2019
Merged

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jun 11, 2019

When running cargo fmt -- --help, no output would be displayed.

Closes rust-lang/cargo#7027.

@ehuss
Copy link
Contributor Author

ehuss commented Jun 11, 2019

This solution is a little sloppy, but I think anything else would require a much larger change.

Copy link
Contributor

@scampi scampi 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 the fix! Can you add a test ?

src/cargo-fmt/main.rs Outdated Show resolved Hide resolved
@ehuss
Copy link
Contributor Author

ehuss commented Jun 12, 2019

Can you add a test ?

Sure. However, there aren't any tests for cargo fmt that I can find. I could set up an integration test to launch it. It won't be completely trivial (it might require things like PATH modification), though. Is that what you're thinking of?

@scampi
Copy link
Contributor

scampi commented Jun 14, 2019

@ehuss re test: I never did so myself, but is it possible to create a method which returns the binary's name, conditionally compiling the method on whether or not a test is run ?

Instead of below, do Command::new(get_binary()):

let mut command = Command::new("rustfmt")

#[cfg(test)]
fn get_binary() -> String {
    // absolute path should be returned
    "./target/debug/rustfmt"
}

#[cfg(not(test))]
fn get_binary() -> String {
    "rustfmt"
}

This would mean that cargo build needs to have run before, but anyway this is already required by some tests:

rustfmt/src/test/mod.rs

Lines 1074 to 1078 in dbac28b

if cfg!(release) {
"no rustfmt bin, try running `cargo build --release` before testing"
} else {
"no rustfmt bin, try running `cargo build` before testing"
}

@ehuss
Copy link
Contributor Author

ehuss commented Jul 6, 2019

I have updated with the different approach, and added some tests. I just used the standard current_exe trick for now. I'm actually working on adding a first-class way for tests to find the binaries in cargo, so hopefully sometime soon that won't be necessary anymore.

Copy link
Contributor

@scampi scampi left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @ehuss, nice tests

use std::env;
use std::process::Command;

/// Run the cargo-fmt executable and return its output.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add comment that the tuple corresponds to (stdout, stderr) of the execution ?

@topecongiro
Copy link
Contributor

Thanks!

@topecongiro topecongiro merged commit e55fc6b into rust-lang:master Jul 14, 2019
@topecongiro topecongiro added this to the 1.3.3 milestone Jul 14, 2019
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.

cargo fmt -- --help displays no output
3 participants