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

Port to clap-cargo #537

Draft
wants to merge 2 commits 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
11 changes: 11 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ toml_edit = { version = "0.4.0", features = ["easy"] }
url = "2.1.1"
ureq = { version = "1.5.1", default-features = false, features = ["tls", "json", "socks"] }
pathdiff = "0.2"
clap-cargo = "0.6.1"
Copy link
Collaborator

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 workflows

Copy link
Author

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 to clap-cargo, as currently nothing prevents you from passing --workspace, --all & -p simultaneously.

Copy link
Collaborator

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.


[dependencies.semver]
features = ["serde"]
Expand Down
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ supported. Git/path dependencies will be ignored.

All packages in the workspace will be upgraded if the `--workspace` flag is supplied.
The `--workspace` flag may be supplied in the presence of a virtual manifest.
Running in workspace root automatically implies `--workspace`

If the '--to-lockfile' flag is supplied, all dependencies will be upgraded to the currently locked
version as recorded in the Cargo.lock file. This flag requires that the Cargo.lock file is
Expand Down
53 changes: 24 additions & 29 deletions src/bin/add/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

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

I'll implement Workspace, Features & Manifest in 3 separate commits, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That works!

Separate PRs are nice because

  • This project is configured for squash-merge only and separate PRs will preserve the distinction
  • If there are easier / harder parts, we can get the easier parts through more quickly, allowing developing more incrementally rather than everything having to be ready at once

Separate commits are nice because

  • Its easier to iterate on all the parts without any blockage due to PR reviews.

Copy link
Author

Choose a reason for hiding this comment

The 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
Expand All @@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This added an all_features that is going unused. I think that will be confusing to users.


/// Do not print any output in case of success.
#[structopt(long = "quiet", short = "q")]
Expand Down Expand Up @@ -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
};
Expand Down Expand Up @@ -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)?,
&registry_url,
)?;
let v = format!(
Expand All @@ -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())
Expand All @@ -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());
}

Expand All @@ -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);
}
Expand Down Expand Up @@ -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(),
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/bin/add/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • This is ignore exclude, all, and workspace arguments
  • This is ignoring all other package values

If it isn't mapping well to add, its ok if we don't use clap_cargo

Copy link
Author

Choose a reason for hiding this comment

The 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 itpackage.get(0) just to make the code compile. It will be changed to processing the whole vector.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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)?;

Expand Down
24 changes: 9 additions & 15 deletions src/bin/rm/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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")]
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto with add

let mut manifest = LocalManifest::find(&manifest_path)?;
let deps = &args.crates;
Expand Down
35 changes: 4 additions & 31 deletions src/bin/set-version/args.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
use std::path::PathBuf;

use structopt::{clap::AppSettings, StructOpt};

#[derive(Debug, StructOpt)]
Expand Down Expand Up @@ -27,38 +25,13 @@ pub(crate) struct Args {
pub metadata: Option<String>,

/// Path to the manifest to upgrade
#[structopt(long = "manifest-path", value_name = "path", conflicts_with = "pkgid")]
pub(crate) manifest_path: Option<PathBuf>,

/// Package id of the crate to change the version of.
#[structopt(
long = "package",
short = "p",
value_name = "pkgid",
conflicts_with = "path",
conflicts_with = "all",
conflicts_with = "workspace"
)]
pub(crate) pkgid: Option<String>,
#[structopt(flatten)]
pub(crate) manifest: clap_cargo::Manifest,

/// Modify all packages in the workspace.
#[structopt(
long = "all",
help = "[deprecated in favor of `--workspace`]",
conflicts_with = "workspace",
conflicts_with = "pkgid"
)]
pub(crate) all: bool,

/// Modify all packages in the workspace.
#[structopt(long = "workspace", conflicts_with = "all", conflicts_with = "pkgid")]
pub(crate) workspace: bool,
#[structopt(flatten)]
pub(crate) workspace: clap_cargo::Workspace,

/// Print changes to be made without making them.
#[structopt(long = "dry-run")]
pub(crate) dry_run: bool,

/// Crates to exclude and not modify.
#[structopt(long)]
pub(crate) exclude: Vec<String>,
}
21 changes: 13 additions & 8 deletions src/bin/set-version/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

The intention of using clap-cargo is we defer to it for interpreting the combination of flags.

See https://docs.rs/clap-cargo/0.6.1/clap_cargo/struct.Workspace.html#method.partition_packages

Copy link
Author

Choose a reason for hiding this comment

The 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.

Expand Down
Loading