feat: cargo profiles inheritance #2595
Conversation
|
@sunshowers could you please take a look at this and give a review when you have a chance? |
| store_dir: Utf8PathBuf, | ||
| default_profile: &'cfg DefaultProfileImpl, | ||
| custom_profile: Option<&'cfg CustomProfileImpl>, | ||
| inheritance_chain: Vec<&'cfg CustomProfileImpl>, |
| // Check custom profile first, then walk up inheritance chain | ||
| if let Some(profile) = self.custom_profile { | ||
| if let Some(retries) = profile.retries { | ||
| return retries; | ||
| } | ||
| } | ||
|
|
||
| // Walk up inheritance chain | ||
| for parent in &self.inheritance_chain { | ||
| if let Some(retries) = parent.retries { | ||
| return retries; | ||
| } | ||
| } | ||
|
|
||
| // Fall back to default | ||
| self.default_profile.retries |
There was a problem hiding this comment.
This looks good -- you'll want to replicate this logic for all of the other config options here (can you extract this into a common function?)
| )); | ||
| } | ||
|
|
||
| visited.insert(profile_name.to_string()); |
There was a problem hiding this comment.
Do you need the to_string here? Or can you store references to the profile name?
You might run into variance issues with storing a reference, but I think you can have another lifetime parameter for that.
| } | ||
|
|
||
| fn check_inheritance_cycles(&self) -> Result<(), ConfigParseError> { | ||
| let mut graph = Graph::<String, (), Directed>::new(); |
There was a problem hiding this comment.
same here, can you store references?
| Err(cycle) => { | ||
| let cycle_profile = graph[cycle.node_id()].clone(); | ||
| Err(ConfigParseError::new( | ||
| "Inheritance cycle detected in profile configuration", |
There was a problem hiding this comment.
We use lowercase for errors:
| "Inheritance cycle detected in profile configuration", | |
| "inheritance cycle detected in profile configuration", |
| Err(ConfigParseError::new( | ||
| "Inheritance cycle detected in profile configuration", | ||
| None, | ||
| ConfigParseErrorKind::InheritanceCycle(cycle_profile), |
There was a problem hiding this comment.
Could we use https://docs.rs/petgraph/latest/petgraph/algo/fn.kosaraju_scc.html to detect all cycles here? Each SCC returned by the algorithm is a cycle -- it would be nice to print out all such cycles here.
| #[serde(default)] | ||
| archive: Option<ArchiveConfig>, | ||
| #[serde(default)] | ||
| inherit: Option<String> |
There was a problem hiding this comment.
Need a trailing comma (did rustfmt not catch this?)
| Ok(()) | ||
| } | ||
|
|
||
| fn check_inheritance_cycles(&self) -> Result<(), ConfigParseError> { |
There was a problem hiding this comment.
We need some tests, both for inheritance and for cycles.
2 participants
No description provided.