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

Allow cargo-fmt to handle formatting individual files. #5071

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 89 additions & 3 deletions src/cargo-fmt/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ pub struct Opts {
)]
packages: Vec<String>,

/// Specify a source file to format
#[clap(short = 's', long = "src-file", value_name = "src-file")]
src_file: Option<PathBuf>,

/// Specify path to Cargo.toml
#[clap(long = "manifest-path", value_name = "manifest-path")]
manifest_path: Option<String>,
Expand Down Expand Up @@ -94,6 +98,28 @@ fn execute() -> i32 {

let opts = Opts::parse_from(args);

if opts.src_file.is_some() & !opts.packages.is_empty() {
print_usage_to_stderr("cannot format source files and packages at the same time");
return FAILURE;
}

if opts.src_file.is_some() & opts.format_all {
print_usage_to_stderr("cannot format all packages when specifying source files");
return FAILURE;
}

if opts.rustfmt_options.iter().any(|s| s.ends_with(".rs")) {
print_usage_to_stderr(
"cannot pass rust files to rustfmt through cargo-fmt. Use '--src-file' instead",
);
return FAILURE;
}

if opts.rustfmt_options.iter().any(|s| s.contains("--edition")) {
print_usage_to_stderr("cannot pass '--edition' to rustfmt through cargo-fmt");
return FAILURE;
}

let verbosity = match (opts.verbose, opts.quiet) {
(false, false) => Verbosity::Normal,
(false, true) => Verbosity::Quiet,
Expand Down Expand Up @@ -319,17 +345,23 @@ pub enum CargoFmtStrategy {
/// Format every packages and dependencies.
All,
/// Format packages that are specified by the command line argument.
Some(Vec<String>),
Packages(Vec<String>),
/// Format the root packages only.
Root,
/// Format individual source files specified by the command line arguments.
SourceFile(PathBuf),
}

impl CargoFmtStrategy {
pub fn from_opts(opts: &Opts) -> CargoFmtStrategy {
if let Some(ref src_file) = opts.src_file {
return CargoFmtStrategy::SourceFile(src_file.clone());
}

match (opts.format_all, opts.packages.is_empty()) {
(false, true) => CargoFmtStrategy::Root,
(true, _) => CargoFmtStrategy::All,
(false, false) => CargoFmtStrategy::Some(opts.packages.clone()),
(false, false) => CargoFmtStrategy::Packages(opts.packages.clone()),
}
}
}
Expand All @@ -346,9 +378,12 @@ fn get_targets(
CargoFmtStrategy::All => {
get_targets_recursive(manifest_path, &mut targets, &mut BTreeSet::new())?
}
CargoFmtStrategy::Some(ref hitlist) => {
CargoFmtStrategy::Packages(ref hitlist) => {
get_targets_with_hitlist(manifest_path, hitlist, &mut targets)?
}
CargoFmtStrategy::SourceFile(ref src_file) => {
get_target_from_src_file(manifest_path, &src_file, &mut targets)?
}
}

if targets.is_empty() {
Expand All @@ -361,6 +396,57 @@ fn get_targets(
}
}

fn get_target_from_src_file(
manifest_path: Option<&Path>,
src_file: &PathBuf,
targets: &mut BTreeSet<Target>,
) -> Result<(), io::Error> {
let metadata = get_cargo_metadata(manifest_path)?;

let get_target = |src_path: &Path| {
metadata
.packages
.iter()
.map(|p| p.targets.iter())
.flatten()
.filter(|t| {
let kind = &t.kind[0];
// to prevent formatting any arbitrary file within the root of our
// project we special case the build.rs script, becuase it's parent
// is the root of the project and we would always select the custom-build
// target in the event that we couldn't find a better target to associate
// with our file.
if kind == "custom-build" && !src_path.ends_with("build.rs") {
return false;
}

if let Ok(target_path) = fs::canonicalize(&t.src_path) {
let target_dir = target_path
.parent()
.expect("Target src_path should have a parent directory");
src_path.starts_with(target_dir)
} else {
false
}
})
.max_by(|t1, t2| {
let t1_len = t1.src_path.components().count();
let t2_len = t2.src_path.components().count();
t1_len.cmp(&t2_len)
})
};

let canonicalize = fs::canonicalize(&src_file)?;
if let Some(target) = get_target(&canonicalize) {
targets.insert(Target {
path: src_file.to_owned(),
kind: target.kind[0].clone(),
edition: target.edition.clone(),
});
}
Ok(())
}

fn get_targets_root_only(
manifest_path: Option<&Path>,
targets: &mut BTreeSet<Target>,
Expand Down
60 changes: 60 additions & 0 deletions src/cargo-fmt/test/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use super::*;
use std::path::PathBuf;

mod message_format;
mod targets;
Expand All @@ -16,6 +17,7 @@ fn default_options() {
assert_eq!(false, o.format_all);
assert_eq!(None, o.manifest_path);
assert_eq!(None, o.message_format);
assert_eq!(None, o.src_file);
}

#[test]
Expand All @@ -27,6 +29,8 @@ fn good_options() {
"p1",
"-p",
"p2",
"-s",
"file.rs",
"--message-format",
"short",
"--check",
Expand All @@ -39,6 +43,7 @@ fn good_options() {
assert_eq!(false, o.version);
assert_eq!(true, o.check);
assert_eq!(vec!["p1", "p2"], o.packages);
assert_eq!(Some(PathBuf::from("file.rs")), o.src_file);
assert_eq!(vec!["--edition", "2018"], o.rustfmt_options);
assert_eq!(false, o.format_all);
assert_eq!(Some(String::from("short")), o.message_format);
Expand Down Expand Up @@ -90,6 +95,25 @@ fn multiple_packages_one_by_one() {
assert_eq!(3, o.packages.len());
}

#[test]
fn cant_list_more_than_one_source_file() {
assert!(
Opts::command()
.try_get_matches_from(&["test", "-s", "src/a.rs", "--src-file", "src/b.rs"])
.is_err()
);
assert!(
!Opts::command()
.try_get_matches_from(&["test", "-s", "src/a.rs"])
.is_err()
);
assert!(
!Opts::command()
.try_get_matches_from(&["test", "--src-file", "src/b.rs"])
.is_err()
);
}

#[test]
fn multiple_packages_grouped() {
let o = Opts::parse_from(&[
Expand Down Expand Up @@ -139,3 +163,39 @@ fn empty_packages_4() {
.is_err()
);
}

#[test]
fn empty_source_files_1() {
assert!(
Opts::command()
.try_get_matches_from(&["test", "-s"])
.is_err()
);
}

#[test]
fn empty_source_files_2() {
assert!(
Opts::command()
.try_get_matches_from(&["test", "-s", "--", "--check"])
.is_err()
);
}

#[test]
fn empty_source_files_3() {
assert!(
Opts::command()
.try_get_matches_from(&["test", "-s", "--verbose"])
.is_err()
);
}

#[test]
fn empty_source_files_4() {
assert!(
Opts::command()
.try_get_matches_from(&["test", "-s", "--check"])
.is_err()
);
}
39 changes: 39 additions & 0 deletions tests/cargo-fmt/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,42 @@ fn cargo_fmt_out_of_line_test_modules() {
assert!(stdout.contains(&format!("Diff in {}", path.display())))
}
}

#[test]
fn cannot_pass_rust_files_to_rustfmt_through_caro_fmt() {
let (_, stderr) = cargo_fmt(&["--", "src/main.rs"]);
assert!(stderr.starts_with(
"cannot pass rust files to rustfmt through cargo-fmt. Use '--src-file' instead"
))
}

#[test]
fn cannot_pass_edition_to_rustfmt_through_caro_fmt() {
let (_, stderr) = cargo_fmt(&["--", "--edition", "2021"]);
assert!(stderr.starts_with("cannot pass '--edition' to rustfmt through cargo-fmt"))
}

#[test]
fn specify_source_files_when_running_cargo_fmt() {
let path = "tests/cargo-fmt/source/workspaces/path-dep-above/e/src/main.rs";
// test that we can run cargo-fmt on a single src file
assert_that!(&["-v", "--check", "-s", path], contains(path));
}

#[test]
fn specify_source_files_in_a_workspace_when_running_cargo_fmt() {
let path = "tests/cargo-fmt/source/workspaces/path-dep-above/ws/a/src/main.rs";
assert_that!(&["-v", "--check", "-s", path], contains(path));
}

#[test]
fn formatting_source_files_and_packages_at_the_same_time_is_not_supported() {
let (_, stderr) = cargo_fmt(&["--check", "-s", "src/main.rs", "-p", "p1"]);
assert!(stderr.starts_with("cannot format source files and packages at the same time"))
}

#[test]
fn formatting_source_files_and_using_package_related_arguments_is_not_supported() {
let (_, stderr) = cargo_fmt(&["--check", "--all", "-s", "src/main.rs"]);
assert!(stderr.starts_with("cannot format all packages when specifying source files"))
}