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

[Gabriel Goller <g.goller@proxmox.com>]

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.)

>+    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.)

>+        } 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.



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel