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

cargo-insta: fix --manifest-path <virtual-workspace> #409

Merged
merged 1 commit into from Oct 7, 2023

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Sep 28, 2023

Prior to this change --manifest-path only worked properly if it pointed to the root of a non-virtual workspace.

Rewrite all the cargo interaction to use cargo-metadata which crucially provides Metadata::resolve::root which is used to determine which package --manifest-path was pointing to.

https://doc.rust-lang.org/cargo/reference/workspaces.html#virtual-workspace

@mitsuhiko
Copy link
Owner

This seems like a good change. A bit of my worry is that we cargo-insta has abysmal test coverage, so I think I would need to do some manual testing on this.

@tamird
Copy link
Contributor Author

tamird commented Sep 28, 2023

Makes sense to me. I did test this locally.

Prior to this change --manifest-path only worked properly if it pointed
to the root of a non-virtual workspace.

Rewrite all the cargo interaction to use cargo-metadata which crucially
provides `Metadata::resolve::root` which is used to determine which
package --manifest-path was pointing to.

https://doc.rust-lang.org/cargo/reference/workspaces.html#virtual-workspace
@tamird
Copy link
Contributor Author

tamird commented Oct 7, 2023

I did find a buglet here in handling a virtual workspace where a manifest path is not specified - it should be fixed now.

@mitsuhiko mitsuhiko merged commit 2e8ceba into mitsuhiko:master Oct 7, 2023
6 checks passed
@tamird tamird deleted the metadata branch October 7, 2023 21:04
@tamird
Copy link
Contributor Author

tamird commented Oct 9, 2023

@mitsuhiko could you cut a new release please?

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.

None yet

2 participants