Feat/improvements#14
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR improves robustness around environment path handling and service version parsing/formatting in src/environment.rs, aiming to eliminate panics and better support non-3-part version strings.
Changes:
- Replace
unwrap()/expect()with fallible error handling in environment path building and service deserialization. - Refactor
Service::get_version()andget_version_without_tag()to build version strings incrementally (and add regression tests for tag formatting). - Minor adjustments to
Serviceserialization version construction.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Path::new("./k8s/environments") | ||
| .join(name) | ||
| .to_str() | ||
| .unwrap() | ||
| .ok_or("Invalid path for environment name")? | ||
| .to_string(), | ||
| ); |
There was a problem hiding this comment.
to_str() returns Option<&str>, and using ok_or("Invalid path for environment name")? yields a Result<_, &str>. In this function the error type is Box<dyn std::error::Error>, and &str does not implement Error, so this ? will not type-check. Convert the failure into a concrete error type (e.g., std::io::Error::other(...)) or map it into a boxed error explicitly before using ?.
| let filemanager = filesystem::FileSystemManager::new( | ||
| Path::new("./k8s/environments") | ||
| .join(&self.name) | ||
| .to_str() | ||
| .unwrap() | ||
| .ok_or("Invalid path for environment name")? | ||
| .to_string(), |
There was a problem hiding this comment.
Same issue as in load_from_file: ok_or("Invalid path for environment name")? produces an &str error, which cannot be converted into the function’s Box<dyn std::error::Error> via ?. Please map this None case into an actual Error (e.g., std::io::Error::other(...)) or otherwise ensure the error type matches the function signature.
| let mut version_str = self.major_version.unwrap_or(0).to_string(); | ||
|
|
||
| if let Some(minor) = self.minor_version { | ||
| version_str.push_str(&format!(".{}", minor)); | ||
| } | ||
|
|
||
| if let Some(patch) = self.patch_version { | ||
| version_str.push_str(&format!(".{}", patch)); | ||
| } | ||
|
|
||
| if let Some(tag) = &self.tag { | ||
| format!( | ||
| "{}.{}.{}-{}", | ||
| self.major_version.unwrap_or(0), | ||
| self.minor_version.unwrap_or(0), | ||
| self.patch_version.unwrap_or(0), | ||
| tag | ||
| ) | ||
| } else { | ||
| // Handle cases where patch_version is None | ||
| if self.patch_version.is_none() { | ||
| format!( | ||
| "{}.{}", | ||
| self.major_version.unwrap_or(0), | ||
| self.minor_version.unwrap_or(0), | ||
| ) | ||
| } else { | ||
| format!( | ||
| "{}.{}.{}", | ||
| self.major_version.unwrap_or(0), | ||
| self.minor_version.unwrap_or(0), | ||
| self.patch_version.unwrap_or(0) | ||
| ) | ||
| } | ||
| version_str.push_str(&format!("-{}", tag)); | ||
| } | ||
|
|
||
| version_str | ||
| } | ||
|
|
||
| pub fn get_version_without_tag(&self) -> String { | ||
| format!( | ||
| "{}.{}.{}", | ||
| self.major_version.unwrap_or(0), // Changed from unwrap() | ||
| self.minor_version.unwrap_or(0), // Changed from unwrap() | ||
| self.patch_version.unwrap_or(0) // Changed from unwrap() | ||
| ) | ||
| let mut version_str = self.major_version.unwrap_or(0).to_string(); | ||
|
|
||
| if let Some(minor) = self.minor_version { | ||
| version_str.push_str(&format!(".{}", minor)); | ||
| } | ||
|
|
||
| if let Some(patch) = self.patch_version { | ||
| version_str.push_str(&format!(".{}", patch)); | ||
| } |
There was a problem hiding this comment.
get_version() / get_version_without_tag() now omit the minor segment when minor_version is None, even if patch_version is Some(_). Because bump_patch_version() can set patch_version while leaving minor_version as None, this can produce ambiguous versions like "1.1" (is that minor=1 or patch=1?) instead of a stable "1.0.1"-style output. Consider enforcing the invariant that patch_version implies minor_version.is_some() (e.g., set minor to 0 when bumping/formatting), or adjust the formatting to include a 0 minor segment whenever a patch segment exists.
No description provided.