Repository navigation
Use cfg_aliases to reduce boilerplate - #2644
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2644 +/- ##
==========================================
- Coverage 78.24% 77.93% -0.31%
==========================================
Files 72 73 +1
Lines 42123 43073 +950
==========================================
+ Hits 32959 33571 +612
- Misses 9164 9502 +338 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
#2581 has 4 more places that would benefit from this patch. |
41782cd to
8275713
Compare
|
The codecov issue is related to codecov/codecov-action#1940 |
640ece9 to
671ccc0
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
| "oss" => self.oss.clone().map(CacheType::OSS).ok_or_else(|| { | ||
| anyhow!("OSS cache not configured but specified in levels") | ||
| })?, | ||
| "disk" => { |
There was a problem hiding this comment.
This block didn't have cos case.
Having a single point of global features gate and cache type addresses it
|
Hey dear @sylvestre Do you think it's worth merging? I feel it would improve the codebase by consolidating all feature-related stuff in a dedicated place. |
| } else { | ||
| return Err(anyhow!( | ||
| let level_lower = level_name.to_lowercase(); | ||
| let (_display_name, cache_type) = |
There was a problem hiding this comment.
display_name is only used in one place.
do we need to return a tuple?
| /// Look up a remote cache type by name (e.g. "s3", "gha"). | ||
| /// Returns `Ok((display_name, Some(..)))` if the name is known and configured, | ||
| /// `Ok((display_name, None))` if known but not configured, `Err` if unknown. | ||
| pub fn cache_type_by_name(&self, name: &str) -> Result<(&'static str, Option<CacheType>)> { |
There was a problem hiding this comment.
this refactor isn't about cfg_aliases.
Please split it into another PR, thanks
There was a problem hiding this comment.
Fair, I left only alias-related changes here and will create a new PR with cache_type_by_name refactoring. I'll address your points there.
| #[cfg(feature = "cos")] | ||
| "cos" => Ok(("COS", self.cos.clone().map(CacheType::COS))), | ||
| _ => bail!( | ||
| "Unknown cache level: '{}' (may require a feature flag, e.g. --features {})", |
There was a problem hiding this comment.
with unknown_cache this suggests --features unknown_cache, which doesn't exist.
and when the feature is missing, the old "requires the 's3' feature" message was clearer, no?
| } | ||
| _ => bail!("Unknown cache level: {}", level_name), | ||
| }; | ||
| if level_name == "disk" { |
There was a problem hiding this comment.
cos is now accepted in the chain, please add a test for it
| .unwrap_err() | ||
| .to_string() | ||
| .contains("S3 cache not configured") | ||
| err.contains("S3 cache not configured") || err.contains("Unknown cache level"), |
There was a problem hiding this comment.
the || makes this test pass for either error.
could you please split it with #[cfg(feature = "s3")] / #[cfg(not(feature = "s3"))]?
|
Thanks for your PR |
|
A follow-up PR #2905 |
cfg_aliases allows to define a long
any(feature = "",....)only once and then reuse it.