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

Conversation

Logarithmus
Copy link

@Logarithmus Logarithmus commented Nov 15, 2021

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
Comment on lines 87 to +89
/// 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,
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.

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.

Comment on lines +123 to 128
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)
};
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)

Comment on lines +100 to 105
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)
};
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

Comment on lines +82 to 90
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)
}?;
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.

Comment on lines 460 to 466
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)
}?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto about using partition_packages

@Logarithmus
Copy link
Author

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

Comment on lines +296 to +304
/// 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() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Surely, it will be replaced with something using partition_packages.

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

@epage
Copy link
Collaborator

epage commented Nov 15, 2021

@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
Copy link
Author

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

@epage
Copy link
Collaborator

epage commented Nov 15, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants