Skip to content

Commit

Permalink
refactor: simplify local dep lookups
Browse files Browse the repository at this point in the history
  • Loading branch information
@calebcartwright
calebcartwright committed Sep 24, 2021
1 parent 7f6229b commit 4b9d637
Showing 1 changed file with 26 additions and 254 deletions.
280 changes: 26 additions & 254 deletions src/cargo-fmt/main.rs
View file
Expand Up @@ -17,6 +17,10 @@ use std::str;

use structopt::StructOpt;

#[path = "test/mod.rs"]
#[cfg(test)]
mod cargo_fmt_tests;

#[derive(StructOpt, Debug)]
#[structopt(
bin_name = "cargo fmt",
Expand Down Expand Up @@ -356,7 +360,7 @@ fn get_targets_root_only(
manifest_path: Option<&Path>,
targets: &mut BTreeSet<Target>,
) -> Result<(), io::Error> {
let metadata = get_cargo_metadata(manifest_path, false)?;
let metadata = get_cargo_metadata(manifest_path)?;
let workspace_root_path = PathBuf::from(&metadata.workspace_root).canonicalize()?;
let (in_workspace_root, current_dir_manifest) = if let Some(target_manifest) = manifest_path {
(
Expand Down Expand Up @@ -400,34 +404,29 @@ fn get_targets_recursive(
mut targets: &mut BTreeSet<Target>,
visited: &mut BTreeSet<String>,
) -> Result<(), io::Error> {
let metadata = get_cargo_metadata(manifest_path, false)?;
let metadata_with_deps = get_cargo_metadata(manifest_path, true)?;

for package in metadata.packages {
let metadata = get_cargo_metadata(manifest_path)?;
for package in &metadata.packages {
add_targets(&package.targets, &mut targets);

// Look for local dependencies.
for dependency in package.dependencies {
if dependency.source.is_some() || visited.contains(&dependency.name) {
// Look for local dependencies using information available since cargo v1.51
// It's theoretically possible someone could use a newer version of rustfmt with
// a much older version of `cargo`, but we don't try to explicitly support that scenario.
// If someone reports an issue with path-based deps not being formatted, be sure to
// confirm their version of `cargo` (not `cargo-fmt`) is >= v1.51
// https://github.com/rust-lang/cargo/pull/8994
for dependency in &package.dependencies {
if dependency.path.is_none() || visited.contains(&dependency.name) {
continue;
}

let dependency_package = metadata_with_deps
.packages
.iter()
.find(|p| p.name == dependency.name && p.source.is_none());
let manifest_path = if let Some(dep_pkg) = dependency_package {
PathBuf::from(&dep_pkg.manifest_path)
} else {
let mut package_manifest_path = PathBuf::from(&package.manifest_path);
package_manifest_path.pop();
package_manifest_path.push(&dependency.name);
package_manifest_path.push("Cargo.toml");
package_manifest_path
};

if manifest_path.exists() {
visited.insert(dependency.name);
let manifest_path = PathBuf::from(dependency.path.as_ref().unwrap()).join("Cargo.toml");
if manifest_path.exists()
&& !metadata
.packages
.iter()
.any(|p| p.manifest_path.eq(&manifest_path))
{
visited.insert(dependency.name.to_owned());
get_targets_recursive(Some(&manifest_path), &mut targets, visited)?;
}
}
Expand All @@ -441,8 +440,7 @@ fn get_targets_with_hitlist(
hitlist: &[String],
targets: &mut BTreeSet<Target>,
) -> Result<(), io::Error> {
let metadata = get_cargo_metadata(manifest_path, false)?;

let metadata = get_cargo_metadata(manifest_path)?;
let mut workspace_hitlist: BTreeSet<&String> = BTreeSet::from_iter(hitlist);

for package in metadata.packages {
Expand Down Expand Up @@ -527,14 +525,9 @@ fn run_rustfmt(
.unwrap_or(SUCCESS))
}

fn get_cargo_metadata(
manifest_path: Option<&Path>,
include_deps: bool,
) -> Result<cargo_metadata::Metadata, io::Error> {
fn get_cargo_metadata(manifest_path: Option<&Path>) -> Result<cargo_metadata::Metadata, io::Error> {
let mut cmd = cargo_metadata::MetadataCommand::new();
if !include_deps {
cmd.no_deps();
}
cmd.no_deps();
if let Some(manifest_path) = manifest_path {
cmd.manifest_path(manifest_path);
}
Expand All @@ -551,224 +544,3 @@ fn get_cargo_metadata(
}
}
}

#[cfg(test)]
mod cargo_fmt_tests {
use super::*;

#[test]
fn default_options() {
let empty: Vec<String> = vec![];
let o = Opts::from_iter(&empty);
assert_eq!(false, o.quiet);
assert_eq!(false, o.verbose);
assert_eq!(false, o.version);
assert_eq!(false, o.check);
assert_eq!(empty, o.packages);
assert_eq!(empty, o.rustfmt_options);
assert_eq!(false, o.format_all);
assert_eq!(None, o.manifest_path);
assert_eq!(None, o.message_format);
}

#[test]
fn good_options() {
let o = Opts::from_iter(&[
"test",
"-q",
"-p",
"p1",
"-p",
"p2",
"--message-format",
"short",
"--check",
"--",
"--edition",
"2018",
]);
assert_eq!(true, o.quiet);
assert_eq!(false, o.verbose);
assert_eq!(false, o.version);
assert_eq!(true, o.check);
assert_eq!(vec!["p1", "p2"], o.packages);
assert_eq!(vec!["--edition", "2018"], o.rustfmt_options);
assert_eq!(false, o.format_all);
assert_eq!(Some(String::from("short")), o.message_format);
}

#[test]
fn unexpected_option() {
assert!(
Opts::clap()
.get_matches_from_safe(&["test", "unexpected"])
.is_err()
);
}

#[test]
fn unexpected_flag() {
assert!(
Opts::clap()
.get_matches_from_safe(&["test", "--flag"])
.is_err()
);
}

#[test]
fn mandatory_separator() {
assert!(
Opts::clap()
.get_matches_from_safe(&["test", "--emit"])
.is_err()
);
assert!(
!Opts::clap()
.get_matches_from_safe(&["test", "--", "--emit"])
.is_err()
);
}

#[test]
fn multiple_packages_one_by_one() {
let o = Opts::from_iter(&[
"test",
"-p",
"package1",
"--package",
"package2",
"-p",
"package3",
]);
assert_eq!(3, o.packages.len());
}

#[test]
fn multiple_packages_grouped() {
let o = Opts::from_iter(&[
"test",
"--package",
"package1",
"package2",
"-p",
"package3",
"package4",
]);
assert_eq!(4, o.packages.len());
}

#[test]
fn empty_packages_1() {
assert!(Opts::clap().get_matches_from_safe(&["test", "-p"]).is_err());
}

#[test]
fn empty_packages_2() {
assert!(
Opts::clap()
.get_matches_from_safe(&["test", "-p", "--", "--check"])
.is_err()
);
}

#[test]
fn empty_packages_3() {
assert!(
Opts::clap()
.get_matches_from_safe(&["test", "-p", "--verbose"])
.is_err()
);
}

#[test]
fn empty_packages_4() {
assert!(
Opts::clap()
.get_matches_from_safe(&["test", "-p", "--check"])
.is_err()
);
}

mod convert_message_format_to_rustfmt_args_tests {
use super::*;

#[test]
fn invalid_message_format() {
assert_eq!(
convert_message_format_to_rustfmt_args("awesome", &mut vec![]),
Err(String::from(
"invalid --message-format value: awesome. Allowed values are: short|json|human"
)),
);
}

#[test]
fn json_message_format_and_check_arg() {
let mut args = vec![String::from("--check")];
assert_eq!(
convert_message_format_to_rustfmt_args("json", &mut args),
Err(String::from(
"cannot include --check arg when --message-format is set to json"
)),
);
}

#[test]
fn json_message_format_and_emit_arg() {
let mut args = vec![String::from("--emit"), String::from("checkstyle")];
assert_eq!(
convert_message_format_to_rustfmt_args("json", &mut args),
Err(String::from(
"cannot include --emit arg when --message-format is set to json"
)),
);
}

#[test]
fn json_message_format() {
let mut args = vec![String::from("--edition"), String::from("2018")];
assert!(convert_message_format_to_rustfmt_args("json", &mut args).is_ok());
assert_eq!(
args,
vec![
String::from("--edition"),
String::from("2018"),
String::from("--emit"),
String::from("json")
]
);
}

#[test]
fn human_message_format() {
let exp_args = vec![String::from("--emit"), String::from("json")];
let mut act_args = exp_args.clone();
assert!(convert_message_format_to_rustfmt_args("human", &mut act_args).is_ok());
assert_eq!(act_args, exp_args);
}

#[test]
fn short_message_format() {
let mut args = vec![String::from("--check")];
assert!(convert_message_format_to_rustfmt_args("short", &mut args).is_ok());
assert_eq!(args, vec![String::from("--check"), String::from("-l")]);
}

#[test]
fn short_message_format_included_short_list_files_flag() {
let mut args = vec![String::from("--check"), String::from("-l")];
assert!(convert_message_format_to_rustfmt_args("short", &mut args).is_ok());
assert_eq!(args, vec![String::from("--check"), String::from("-l")]);
}

#[test]
fn short_message_format_included_long_list_files_flag() {
let mut args = vec![String::from("--check"), String::from("--files-with-diff")];
assert!(convert_message_format_to_rustfmt_args("short", &mut args).is_ok());
assert_eq!(
args,
vec![String::from("--check"), String::from("--files-with-diff")]
);
}
}
}

0 comments on commit 4b9d637

Please sign in to comment.