From f4f5187e7dc9b9c177e099bbf7f3f42556867328 Mon Sep 17 00:00:00 2001 From: "C. Titus Brown" Date: Wed, 18 Dec 2024 10:20:43 -0800 Subject: [PATCH] MRG: adjust `Signature::name()` to return `Option` instead of `filename()` and `md5sum()` (#3434) This PR adjusts `Signature::name()` to return `None` when no name is set, instead of returning first `filename()` or (if empty) `md5sum()`. It also adds `name_str()` which returns an empty string, to avoid too many `unwrap_or` scattered throughout the codebase. Fixes https://github.com/sourmash-bio/sourmash/issues/3441 --- src/core/src/index/revindex/disk_revindex.rs | 9 ++++++++- src/core/src/index/revindex/mod.rs | 2 +- src/core/src/manifest.rs | 2 +- src/core/src/signature.rs | 17 +++++++++-------- src/core/src/storage/mod.rs | 4 ++-- src/core/tests/storage.rs | 2 +- 6 files changed, 22 insertions(+), 14 deletions(-) diff --git a/src/core/src/index/revindex/disk_revindex.rs b/src/core/src/index/revindex/disk_revindex.rs index 46552c2c6..1a81a6208 100644 --- a/src/core/src/index/revindex/disk_revindex.rs +++ b/src/core/src/index/revindex/disk_revindex.rs @@ -351,7 +351,14 @@ impl RevIndexOps for RevIndex { .collection .record_for_dataset(dataset_id) .expect("dataset not found"); - Some((row.name().into(), size)) + + let name = [row.name(), row.filename(), row.md5()] + .into_iter() + .skip_while(|v| v.is_empty()) + .next() + .unwrap(); // guaranteed to succeed because `md5` always exists + + Some((name.into(), size)) } else { None } diff --git a/src/core/src/index/revindex/mod.rs b/src/core/src/index/revindex/mod.rs index f1248be71..606e6b9c2 100644 --- a/src/core/src/index/revindex/mod.rs +++ b/src/core/src/index/revindex/mod.rs @@ -558,7 +558,7 @@ mod test { )?; assert_eq!(matches.len(), 1); - assert_eq!(matches[0].name(), "../genome-s10.fa.gz"); + assert_eq!(matches[0].name(), ""); // signature name is empty assert_eq!(matches[0].f_match(), 1.0); Ok(()) diff --git a/src/core/src/manifest.rs b/src/core/src/manifest.rs index 21f8ecbc5..4a49808f6 100644 --- a/src/core/src/manifest.rs +++ b/src/core/src/manifest.rs @@ -129,7 +129,7 @@ impl Record { Self { internal_location: path.into(), moltype: moltype.to_string(), - name: sig.name(), + name: sig.name_str(), ksize, md5, md5short, diff --git a/src/core/src/signature.rs b/src/core/src/signature.rs index 31fc86f4a..5a0d39f61 100644 --- a/src/core/src/signature.rs +++ b/src/core/src/signature.rs @@ -445,14 +445,13 @@ fn default_version() -> f64 { } impl Signature { - pub fn name(&self) -> String { - if let Some(name) = &self.name { - name.clone() - } else if let Some(filename) = &self.filename { - filename.clone() - } else { - self.md5sum() - } + pub fn name(&self) -> Option { + self.name.clone() + } + + /// return name, if not None; or "" if None. + pub fn name_str(&self) -> String { + self.name().unwrap_or("".into()) } pub fn set_name(&mut self, name: &str) { @@ -982,6 +981,8 @@ mod test { assert_eq!(sig.signatures[0].size(), 3); assert_eq!(sig.signatures[1].size(), 2); assert_eq!(sig.signatures[2].size(), 1); + + assert_eq!(sig.name_str(), ""); } #[test] diff --git a/src/core/src/storage/mod.rs b/src/core/src/storage/mod.rs index b026b80d4..94b5fb815 100644 --- a/src/core/src/storage/mod.rs +++ b/src/core/src/storage/mod.rs @@ -466,7 +466,7 @@ impl ZipStorage { impl SigStore { pub fn new_with_storage(sig: Signature, storage: InnerStorage) -> Self { - let name = sig.name(); + let name = sig.name_str(); let filename = sig.filename(); SigStore::builder() @@ -555,7 +555,7 @@ impl Deref for SigStore { impl From for SigStore { fn from(other: Signature) -> SigStore { - let name = other.name(); + let name = other.name_str(); let filename = other.filename(); SigStore::builder() diff --git a/src/core/tests/storage.rs b/src/core/tests/storage.rs index e0d355d6b..68a04ccc7 100644 --- a/src/core/tests/storage.rs +++ b/src/core/tests/storage.rs @@ -98,7 +98,7 @@ fn innerstorage_save_sig() -> Result<(), Box> { let loaded_sig = instorage.load_sig("test")?; - assert_eq!(sig.name(), loaded_sig.name()); + assert_eq!(sig.name_str(), loaded_sig.name()); assert_eq!(sig.md5sum(), loaded_sig.md5sum()); Ok(())