[pbs-devel] [PATCH proxmox-backup 2/4] api types: introduce `BackupArchiveName` type

[Christian Ebner]

> On 09.07.2024 11:37 CEST Gabriel Goller <g.goller at proxmox.com> wrote:
> 
>  
> Doesn't apply on master anymore, but here some quick things I noticed
> skimming over.
> 
> On 04.07.2024 10:55, Christian Ebner wrote:
> >diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> >index b63e7c2ff..16c3eef59 100644
> >--- a/pbs-api-types/src/datastore.rs
> >+++ b/pbs-api-types/src/datastore.rs
> >+impl std::fmt::Display for BackupArchiveName {
> 
> I think we can import the std::* traits here (and below as well.)

Okay, fine by me, will use the already imported fmt module for traits located in there instead, to be in line with the current code and import other traits at the beginning.

> 
> >+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> >+        write!(f, "{name}", name = self.name)
> >+    }
> >+}
> >+
> >+serde_plain::derive_deserialize_from_fromstr!(BackupArchiveName, "archive name");
> >+
> >+impl std::str::FromStr for BackupArchiveName {
> >+    type Err = Error;
> >+
> >+    fn from_str(name: &str) -> Result<Self, Self::Err> {
> >+        Self::try_from(name)
> >+    }
> >+}
> >+
> >+serde_plain::derive_serialize_from_display!(BackupArchiveName);
> >+
> >+impl std::convert::TryFrom<&str> for BackupArchiveName {
> >+    type Error = anyhow::Error;
> >+
> >+    fn try_from(value: &str) -> Result<Self, Self::Error> {
> >+        let (name, ty) = Self::parse_archive_type(value)?;
> >+        Ok(Self { name, ty })
> >+    }
> >+}
> >+
> >+impl std::convert::AsRef<str> for BackupArchiveName {
> >+    fn as_ref(&self) -> &str {
> >+        &self.name
> >+    }
> >+}
> >+
> >+impl BackupArchiveName {
> >+    pub fn from_path(path: impl AsRef<Path>) -> Result<Self, Error> {
> >+        let path = path.as_ref();
> >+        let file_name = path
> >+            .file_name()
> >+            .ok_or_else(|| format_err!("invalid archive name"))?;
> >+        let name = file_name
> >+            .to_str()
> >+            .ok_or_else(|| format_err!("archive name not valid UTF-8"))?;
> >+
> >+        Self::try_from(name)
> >+    }
> >+
> >+    pub fn archive_type(&self) -> ArchiveType {
> >+        self.ty.clone()
> >+    }
> >+
> >+    pub fn ends_with(&self, postfix: &str) -> bool {
> >+        self.name.ends_with(postfix)
> >+    }
> >+
> >+    pub fn has_pxar_filename_extension(&self) -> bool {
> >+        self.name.ends_with(".pxar.didx")
> >+            || self.name.ends_with(".mpxar.didx")
> >+            || self.name.ends_with(".ppxar.didx")
> >+    }
> >+
> >+    pub fn without_type_extension(&self) -> String {
> >+        match self.ty {
> >+            ArchiveType::DynamicIndex => self.name.strip_suffix(".didx").unwrap().into(),
> >+            ArchiveType::FixedIndex => self.name.strip_suffix(".fidx").unwrap().into(),
> >+            ArchiveType::Blob => self.name.strip_suffix(".blob").unwrap().into(),
> >+        }
> >+    }
> >+
> >+    fn parse_archive_type(archive_name: &str) -> Result<(String, ArchiveType), Error> {
> >+        if archive_name.ends_with(".didx")
> >+            || archive_name.ends_with(".fidx")
> >+            || archive_name.ends_with(".blob")
> >+        {
> >+            Ok((archive_name.into(), ArchiveType::from_path(archive_name)?))
> >+        } else if archive_name.ends_with(".pxar")
> >+            || archive_name.ends_with(".mpxar")
> >+            || archive_name.ends_with(".ppxar")
> >+        {
> >+            Ok((format!("{}.didx", archive_name), ArchiveType::DynamicIndex))
> 
> `archive_name` can be inlined here (below as well.)

Will be inlined in v2.

> 
> >+        } else if archive_name.ends_with(".img") {
> >+            Ok((format!("{}.fidx", archive_name), ArchiveType::FixedIndex))
> >+        } else {
> >+            Ok((format!("{}.blob", archive_name), ArchiveType::Blob))
> >+        }
> >+    }
> >+}
> >+
> >+impl ApiType for BackupArchiveName {
> >+    const API_SCHEMA: Schema = BACKUP_ARCHIVE_NAME_SCHEMA;
> >+}
> 
> Everything quite straightforward, but we could throw in some unit-tests
> as it won't do us any harm.

Sure, will add tests covering the relevant cases.