-
Notifications
You must be signed in to change notification settings - Fork 153
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
Port to clap-cargo
#537
base: master
Are you sure you want to change the base?
Port to clap-cargo
#537
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,7 +35,7 @@ pub struct Args { | |
pub crates: Vec<String>, | ||
|
||
/// Rename a dependency in Cargo.toml, | ||
/// https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#renaming-dependencies-in-cargotoml. | ||
/// <https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#renaming-dependencies-in-cargotoml>. | ||
/// Only works when specifying a single dependency. | ||
#[structopt(long = "rename", short = "r")] | ||
pub rename: Option<String>, | ||
|
@@ -85,17 +85,11 @@ pub struct Args { | |
pub optional: bool, | ||
|
||
/// Path to the manifest to add a dependency to. | ||
#[structopt(long = "manifest-path", value_name = "path", conflicts_with = "pkgid")] | ||
pub manifest_path: Option<PathBuf>, | ||
#[structopt(flatten)] | ||
pub manifest: clap_cargo::Manifest, | ||
Comment on lines
87
to
+89
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you focus the PR, or at least the commits, to only porting to one clap_cargo feature at a time? It'll make it easier to see the impact. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll implement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That works! Separate PRs are nice because
Separate commits are nice because
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @epage I'll work in this PR for now, maybe later I'll change my mind & file another 2 PRs. |
||
|
||
/// Package id of the crate to add this dependency to. | ||
#[structopt( | ||
long = "package", | ||
short = "p", | ||
value_name = "pkgid", | ||
conflicts_with = "path" | ||
epage marked this conversation as resolved.
Show resolved
Hide resolved
|
||
)] | ||
pub pkgid: Option<String>, | ||
#[structopt(flatten)] | ||
pub workspace: clap_cargo::Workspace, | ||
|
||
/// Choose method of semantic version upgrade. Must be one of "none" (exact version, `=` | ||
/// modifier), "patch" (`~` modifier), "minor" (`^` modifier), "all" (`>=`), or "default" (no | ||
|
@@ -117,14 +111,9 @@ pub struct Args { | |
#[structopt(long = "allow-prerelease")] | ||
pub allow_prerelease: bool, | ||
|
||
/// Space-separated list of features to add. For an alternative approach to | ||
/// enabling features, consider installing the `cargo-feature` utility. | ||
#[structopt(long = "features", number_of_values = 1)] | ||
pub features: Option<Vec<String>>, | ||
|
||
/// Set `default-features = false` for the added dependency. | ||
#[structopt(long = "no-default-features")] | ||
pub no_default_features: bool, | ||
/// For an alternative approach to enabling features, consider installing `cargo-feature`. | ||
#[structopt(flatten)] | ||
pub features: clap_cargo::Features, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This added an |
||
|
||
/// Do not print any output in case of success. | ||
#[structopt(long = "quiet", short = "q")] | ||
|
@@ -229,7 +218,10 @@ impl Args { | |
dependency = dependency.set_version(parse_version_req(version)?); | ||
} | ||
let registry_url = if let Some(registry) = &self.registry { | ||
Some(registry_url(&find(&self.manifest_path)?, Some(registry))?) | ||
Some(registry_url( | ||
&find(&self.manifest.manifest_path)?, | ||
Some(registry), | ||
)?) | ||
} else { | ||
None | ||
}; | ||
|
@@ -262,7 +254,7 @@ impl Args { | |
dependency = get_latest_dependency( | ||
crate_name.name(), | ||
self.allow_prerelease, | ||
&find(&self.manifest_path)?, | ||
&find(&self.manifest.manifest_path)?, | ||
®istry_url, | ||
)?; | ||
let v = format!( | ||
|
@@ -287,7 +279,7 @@ impl Args { | |
|
||
/// Build dependencies from arguments | ||
pub fn parse_dependencies(&self) -> Result<Vec<Dependency>> { | ||
let workspace_members = workspace_members(self.manifest_path.as_deref())?; | ||
let workspace_members = workspace_members(self.manifest.manifest_path.as_deref())?; | ||
|
||
if self.crates.len() > 1 | ||
&& (self.git.is_some() || self.path.is_some() || self.vers.is_some()) | ||
|
@@ -299,7 +291,7 @@ impl Args { | |
return Err(ErrorKind::MultipleCratesWithRename.into()); | ||
} | ||
|
||
if self.crates.len() > 1 && self.features.is_some() { | ||
if self.crates.len() > 1 && !self.features.features.is_empty() { | ||
return Err(ErrorKind::MultipleCratesWithFeatures.into()); | ||
} | ||
|
||
|
@@ -310,8 +302,12 @@ impl Args { | |
.map(|x| { | ||
let mut x = x | ||
.set_optional(self.optional) | ||
.set_features(self.features.clone()) | ||
.set_default_features(!self.no_default_features); | ||
.set_features(if self.features.features.is_empty() { | ||
None | ||
} else { | ||
Some(self.features.features.clone()) | ||
}) | ||
.set_default_features(!self.features.no_default_features); | ||
if let Some(ref rename) = self.rename { | ||
x = x.set_rename(rename); | ||
} | ||
|
@@ -347,16 +343,15 @@ impl Default for Args { | |
path: None, | ||
target: None, | ||
optional: false, | ||
manifest_path: None, | ||
pkgid: None, | ||
upgrade: "minor".to_string(), | ||
allow_prerelease: false, | ||
features: None, | ||
no_default_features: false, | ||
quiet: false, | ||
offline: true, | ||
sort: false, | ||
registry: None, | ||
manifest: Default::default(), | ||
workspace: Default::default(), | ||
features: Default::default(), | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -120,11 +120,11 @@ fn is_sorted(mut it: impl Iterator<Item = impl PartialOrd>) -> bool { | |
} | ||
|
||
fn handle_add(args: &Args) -> Result<()> { | ||
let manifest_path = if let Some(ref pkgid) = args.pkgid { | ||
let pkg = manifest_from_pkgid(args.manifest_path.as_deref(), pkgid)?; | ||
let manifest_path = if let Some(pkgid) = args.workspace.package.get(0) { | ||
let pkg = manifest_from_pkgid(args.manifest.manifest_path.as_deref(), pkgid)?; | ||
Cow::Owned(Some(pkg.manifest_path.into_std_path_buf())) | ||
} else { | ||
Cow::Borrowed(&args.manifest_path) | ||
Cow::Borrowed(&args.manifest.manifest_path) | ||
}; | ||
Comment on lines
+123
to
128
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If it isn't mapping well to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @epage I think adding the same dependency to multiple crates in a workspace could be useful. I've made it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be worth breaking that out into its own PR, after this one is done, so we can focus on one user-facing aspect at a time (existing multi-crate behavior vs making another command multi-crate) |
||
let mut manifest = LocalManifest::find(&manifest_path)?; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,7 @@ extern crate error_chain; | |
use cargo_edit::{manifest_from_pkgid, LocalManifest}; | ||
use std::borrow::Cow; | ||
use std::io::Write; | ||
use std::path::PathBuf; | ||
|
||
use std::process; | ||
use structopt::{clap::AppSettings, StructOpt}; | ||
use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor}; | ||
|
@@ -58,17 +58,11 @@ struct Args { | |
build: bool, | ||
|
||
/// Path to the manifest to remove a dependency from. | ||
#[structopt(long = "manifest-path", value_name = "path", conflicts_with = "pkgid")] | ||
manifest_path: Option<PathBuf>, | ||
|
||
/// Package id of the crate to remove this dependency from. | ||
#[structopt( | ||
long = "package", | ||
short = "p", | ||
value_name = "pkgid", | ||
conflicts_with = "path" | ||
)] | ||
pkgid: Option<String>, | ||
#[structopt(flatten)] | ||
manifest: clap_cargo::Manifest, | ||
|
||
#[structopt(flatten)] | ||
workspace: clap_cargo::Workspace, | ||
|
||
/// Do not print any output in case of success. | ||
#[structopt(long = "quiet", short = "q")] | ||
|
@@ -103,11 +97,11 @@ fn print_msg(name: &str, section: &str) -> Result<()> { | |
} | ||
|
||
fn handle_rm(args: &Args) -> Result<()> { | ||
let manifest_path = if let Some(ref pkgid) = args.pkgid { | ||
let pkg = manifest_from_pkgid(args.manifest_path.as_deref(), pkgid)?; | ||
let manifest_path = if !args.workspace.package.is_empty() { | ||
let pkg = manifest_from_pkgid(args.manifest.manifest_path.as_deref(), &args.workspace.package[0])?; | ||
Cow::Owned(Some(pkg.manifest_path.into_std_path_buf())) | ||
} else { | ||
Cow::Borrowed(&args.manifest_path) | ||
Cow::Borrowed(&args.manifest.manifest_path) | ||
}; | ||
Comment on lines
+100
to
105
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto with |
||
let mut manifest = LocalManifest::find(&manifest_path)?; | ||
let deps = &args.crates; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,12 +57,16 @@ fn process(args: Args) -> Result<()> { | |
target, | ||
bump, | ||
metadata, | ||
manifest_path, | ||
pkgid, | ||
all, | ||
manifest: clap_cargo::Manifest { manifest_path, .. }, | ||
workspace: | ||
clap_cargo::Workspace { | ||
package: pkgid, | ||
workspace, | ||
all, | ||
exclude, | ||
.. | ||
}, | ||
dry_run, | ||
workspace, | ||
exclude, | ||
} = args; | ||
|
||
let target = match (target, bump) { | ||
|
@@ -75,11 +79,12 @@ fn process(args: Args) -> Result<()> { | |
if all { | ||
deprecated_message("The flag `--all` has been deprecated in favor of `--workspace`")?; | ||
} | ||
let all = workspace || all; | ||
let all = workspace || all || LocalManifest::find(&None)?.is_virtual(); | ||
|
||
let manifests = if all { | ||
Manifests::get_all(&manifest_path) | ||
} else if let Some(ref pkgid) = pkgid { | ||
Manifests::get_pkgid(manifest_path.as_deref(), pkgid) | ||
} else if let Some(id) = pkgid.get(0) { | ||
Manifests::get_pkgid(manifest_path.as_deref(), id) | ||
} else { | ||
Manifests::get_local_one(&manifest_path) | ||
}?; | ||
Comment on lines
+82
to
90
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The intention of using See https://docs.rs/clap-cargo/0.6.1/clap_cargo/struct.Workspace.html#method.partition_packages There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I've seen this function & indend to use it, but as I've said this PR is in early stage. I've created it just to make sure other people know I'm working on it & wouldn't do duplicate work. Marked it draft & placed "work in progress" notice in the body. Anyway, thanks for your review comments. |
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just released 0.7 to add the
-p
flag which was missing. We'll want this to not have a regression in people's workflowsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@epage we probably should also add
structopt...conflicts_with
toclap-cargo
, as currently nothing prevents you from passing--workspace
,--all
&-p
simultaneously.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not conflicting in cargo so by using
clap-cargo
, we'll be better aligning in our behavior in this way as well.