Port to `clap-cargo` by Logarithmus · Pull Request #537 · killercup/cargo-edit

Logarithmus · 2021-11-15T01:06:17Z

Work in progress. See #535 (comment)

Imply --workspace when running in workspace root
Port to clap-cargo

Aims for saner defaults & to match `cargo update` behavior

epage · 2021-11-15T15:19:23Z

src/bin/add/args.rs

    /// 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,


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.

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

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.

@epage I'll work in this PR for now, maybe later I'll change my mind & file another 2 PRs.

src/bin/add/args.rs

epage · 2021-11-15T15:26:32Z

src/bin/add/args.rs

-    pub no_default_features: bool,
+    /// For an alternative approach to enabling features, consider installing `cargo-feature`.
+    #[structopt(flatten)]
+    pub features: clap_cargo::Features,


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

epage · 2021-11-15T15:36:40Z

src/bin/add/main.rs

+    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)
    };


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

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

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)

epage · 2021-11-15T15:37:44Z

src/bin/rm/main.rs

+    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)
    };


ditto with add

epage · 2021-11-15T15:39:27Z

src/bin/set-version/main.rs

+    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)
    }?;


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

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.

epage · 2021-11-15T15:40:17Z

src/bin/upgrade/main.rs

    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 !pkgid.is_empty() {
+        Manifests::get_pkgid(manifest_path.as_deref(), &pkgid[0])
    } else {
        Manifests::get_local_one(&manifest_path)
    }?;


ditto about using partition_packages

Logarithmus · 2021-11-15T15:41:44Z

@epage this PR is in very early stage, so don't expect too much 🙂

epage · 2021-11-15T15:42:06Z

src/manifest.rs

+    /// Check if local manifest is virtual, i. e. corresponds to workspace root
+    pub fn is_virtual(&self) -> bool {
+        !self.data["workspace"].is_none()
+    }
+
    /// Write changes back to the file
    pub fn write(&self) -> Result<()> {
        if self.manifest.data["package"].is_none() && self.manifest.data["project"].is_none() {
-            if !self.manifest.data["workspace"].is_none() {
+            if self.is_virtual() {


Please revert this. The function is_virtual, on its own, isn't correctly reporting virtual packages, only workspace roots. Its only virtual when combined with the preceding checks

Surely, it will be replaced with something using partition_packages.

epage · 2021-11-15T15:42:57Z

Cargo.toml

 url = "2.1.1"
 ureq = { version = "1.5.1", default-features = false, features = ["tls", "json", "socks"] }
 pathdiff = "0.2"
+clap-cargo = "0.6.1"


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

@epage we probably should also add structopt...conflicts_with to clap-cargo, as currently nothing prevents you from passing --workspace, --all & -p simultaneously.

These are not conflicting in cargo so by using clap-cargo, we'll be better aligning in our behavior in this way as well.

epage · 2021-11-15T15:44:12Z

@epage this PR is in very early stage, so don't expect too much slightly_smiling_face

Thats fine; just figured it'd help to give coordinate along he way so we can catch problems early (like me needing to fix some things in clap-cargo)

Logarithmus · 2021-11-15T15:52:23Z

@epage I've just noticed you are the author of clap-cargo. Didn't really see it before.

epage · 2021-11-15T16:01:15Z

@epage I've just noticed you are the author of clap-cargo. Didn't really see it before.

Yeah, I created it after cargo-edit was created and long before I got involved in cargo-edit. When I added cargo-set-version, I focused on following how things currently worked rather than also introducing clap-cargo.

Imply --workspace when running in workspace root

28ee3f7

Aims for saner defaults & to match `cargo update` behavior

Logarithmus marked this pull request as draft November 15, 2021 01:07

Logarithmus mentioned this pull request Nov 15, 2021

Imply --workspace when running in workspace root #535

Closed

Port to clap-cargo

aa5dfc7

Logarithmus force-pushed the feat/clap-cargo branch from af5718a to aa5dfc7 Compare November 15, 2021 01:39

epage reviewed Nov 15, 2021

View reviewed changes

src/bin/add/args.rs Show resolved Hide resolved

epage reviewed Nov 15, 2021

View reviewed changes

Conversation

Logarithmus commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Logarithmus commented Nov 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epage commented Nov 15, 2021

Uh oh!

Logarithmus commented Nov 15, 2021

Uh oh!

epage commented Nov 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Logarithmus commented Nov 15, 2021 •

edited

Loading