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

Add workspace_default_members field #248

Merged
merged 2 commits into from Sep 6, 2023

Conversation

sorcio
Copy link
Contributor

@sorcio sorcio commented Aug 23, 2023

Fixes #215

  • Adding new field workspace_default_members which was added in Cargo 1.71. I made it an option because we have no way to reconstruct the value in previous Cargo versions.
  • I also added a convenience method to collect &Packages that are default workspace members. But it also needs to either return an Option or panic. I'm not sure this is ideal, because at some point in the future the minimum supported Cargo version might be >=1.71 and changing this API would be a backwards incompatible change. But panicking doesn't feel ideal either. Should I remove this method?
  • I did not want to bump the minimum version for the all_the_fields test. Instead I added a conditional assertion, following the same pattern introduced by the rust_version field.
  • There are no tests that exercise this field other than "it should be the same as workspace_members". Should I add some test data?

src/lib.rs Outdated Show resolved Hide resolved
@sorcio
Copy link
Contributor Author

sorcio commented Aug 31, 2023

@oli-obk ok, I tried the "panic on deref" solution.

I had decided not to add any public API to inspect whether workspace_default_members has a valid value—basically delegating to users to only use it with the right version of cargo. But this got annoying in tests, so I did an ugly and exposed the check as an undocumented pub fn1.

Unrelated to this PR: would it be interesting to expose something similar in spirit to cargo_version() as public API?

Footnotes

  1. the reason it's a separate function with a quirky name and not a method is a maybe excessive concern with namespace clashing.

@oli-obk
Copy link
Owner

oli-obk commented Aug 31, 2023

would it be interesting to expose something similar in spirit to cargo_version() as public API?

I don't know where we'd get this information from and how we'd use it. Do you have ideas?

@sorcio
Copy link
Contributor Author

sorcio commented Aug 31, 2023

would it be interesting to expose something similar in spirit to cargo_version() as public API?

I don't know where we'd get this information from and how we'd use it. Do you have ideas?

I didn't think too hard about it, but MetadataCommand already needs to identify a cargo executable and the output from cargo --version is parsable (probably in all supported versions till today?). Would that require to ask cargo to stabilize the output format?

The information might be useful to cargo plugins and libraries that support plugins, in order to gracefully degrade when certain features are not available. I don't have a specific use case, though!

@oli-obk
Copy link
Owner

oli-obk commented Sep 6, 2023

There's already a crate for this: https://crates.io/crates/rustc_version

I think users can combine cargo_metadata and rustc_version in their own crates, instead of us combining it for them here

@oli-obk oli-obk merged commit fd72a0b into oli-obk:main Sep 6, 2023
6 checks passed
@sorcio sorcio deleted the add_worskpace_default_members branch September 6, 2023 13:21
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.

workspace.default-members key is missing from output
2 participants