feat: add features parameter to cache_crate for mutually exclusive features#57
feat: add features parameter to cache_crate for mutually exclusive features#57bpodwinski wants to merge 5 commits intosnowmead:mainfrom
Conversation
snowmead
left a comment
snowmead
left a comment
There was a problem hiding this comment.
Thanks for the change!
I noticed there are a few places not covered in some of the crate caching paths for accepting a feature set. Please make sure all of them can accept features. That way some tools don't download all the features by default.
|
|
||
| /// Get a description of this strategy for logging | ||
| fn description(&self) -> &str { | ||
| fn description(&self) -> String { |
There was a problem hiding this comment.
I don't see why this needs to return a String instead of &str
| /// Run cargo rustdoc with JSON output for a crate or specific package | ||
| /// | ||
| /// # Parameters | ||
| /// - `source_path`: The root directory containing Cargo.toml | ||
| /// - `package`: Optional package name for workspace members | ||
| /// - `target_dir`: Optional custom target directory to avoid conflicts when building | ||
| /// multiple workspace members concurrently. When building multiple workspace members | ||
| /// in parallel, each must use a unique target directory to prevent cargo from | ||
| /// conflicting with itself. See [`DocGenerator::generate_workspace_member_docs`](crate::cache::docgen::DocGenerator::generate_workspace_member_docs) | ||
| /// for the implementation pattern. |
There was a problem hiding this comment.
All these docs should be above run_cargo_rustdoc_json
| fn build_feature_strategies(features: Option<Vec<String>>) -> Vec<FeatureStrategy> { | ||
| if let Some(feats) = features { | ||
| vec![ | ||
| FeatureStrategy::Specific(feats), | ||
| FeatureStrategy::DefaultFeatures, | ||
| FeatureStrategy::NoDefaultFeatures, | ||
| ] | ||
| } else { | ||
| vec![ | ||
| FeatureStrategy::AllFeatures, | ||
| FeatureStrategy::DefaultFeatures, | ||
| FeatureStrategy::NoDefaultFeatures, | ||
| ] | ||
| } | ||
| } | ||
|
|
| // Try different feature strategies in order | ||
| let strategies = [ | ||
| FeatureStrategy::AllFeatures, | ||
| FeatureStrategy::DefaultFeatures, | ||
| FeatureStrategy::NoDefaultFeatures, | ||
| ]; |
There was a problem hiding this comment.
we could just add an if features is specified, add it to the array, simpler to reason about
There was a problem hiding this comment.
this would require removing all the tests below
There was a problem hiding this comment.
We should add actual integration tests for to make sure specifying features works
2 participants
Summary
featuresparameter tocache_crateallowing users tospecify exact Cargo features instead of
--all-featuresFeatureStrategy::Specific(Vec<String>)variant inrustdoc.rswith fallback order:
Specific → DefaultFeatures → NoDefaultFeaturesleptos-usewithaxumvsactix)README.mdandinstall.shwith current tool list (cache_crateunified tool,
cache_operations)Test plan
FeatureStrategy::Specific— args and descriptioncargo fmtandcargo clippypasscache_crate({crate_name: "leptos-use", source_type: "cratesio",
version: "0.15.8", features: ["axum"]})
featuresis omitted