MRG: adjust `Signature::name()` to return `Option<String>` instead of `filename()` and `md5sum()` #3434

ctb · 2024-12-15T21:03:39Z

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 #3441

codecov · 2024-12-15T21:11:58Z

Codecov Report

Attention: Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 86.42%. Comparing base (635ffc1) to head (9f0702d).
Report is 1 commits behind head on latest.

Files with missing lines	Patch %	Lines
src/core/src/index/revindex/disk_revindex.rs	33.33%	2 Missing ⚠️
src/core/src/signature.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           latest    #3434      +/-   ##
==========================================
- Coverage   86.44%   86.42%   -0.03%     
==========================================
  Files         137      137              
  Lines       16102    16106       +4     
  Branches     2219     2219              
==========================================
- Hits        13920    13919       -1     
- Misses       1875     1880       +5     
  Partials      307      307

Flag	Coverage Δ
hypothesis-py	`25.43% <ø> (ø)`
python	`92.40% <ø> (ø)`
rust	`62.11% <62.50%> (-0.11%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…tersect_manifest

ctb · 2024-12-17T01:18:51Z

src/core/src/index/revindex/disk_revindex.rs

@@ -351,7 +351,15 @@ impl RevIndexOps for RevIndex {
                        .collection
                        .record_for_dataset(dataset_id)
                        .expect("dataset not found");
-                    Some((row.name().into(), size))
+
+                    let mut name = row.name();


This is ugly - is there an idiomatic way to do this??

yuck: https://stackoverflow.com/questions/45343320/how-can-i-return-the-first-non-empty-string

maybe?

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

in 0ed80c1

ctb · 2024-12-17T02:25:33Z

@luizirber @bluegenes ready for review!

luizirber · 2024-12-17T18:19:44Z

src/core/src/signature.rs

@@ -448,10 +448,8 @@ impl Signature {
    pub fn name(&self) -> String {


since r0.18.0 is not released yet, do we want to make this
pub fn name(&self) -> Option<String> {
instead? This wil change calls around in other places, but seems to communicate better that the name exists or not instead of getting a .name() call that might return ""

ooh, I like it! let me give it a try 🙇

ctb · 2024-12-17T20:55:15Z

ready for review @luizirber !

src/core/src/signature.rs

## [0.18.0] - 2024-12-20 MSRV: 1.66 Changes/additions: * add skipmer capacity to sourmash python layer via ffi (#3446) * add skipmers; switch to reading frame approach for translation, skipmers (#3395) * Refactor: Use to_writer/from_reader across the codebase (#3443) * adjust `Signature::name()` to return `Option<String>` instead of `filename()` and `md5sum()` (#3434) * propagate zipfile errors (#3431) Updates: * Bump proptest from 1.5.0 to 1.6.0 (#3437) * Bump roaring from 0.10.8 to 0.10.9 (#3438) * Bump serde from 1.0.215 to 1.0.216 (#3436) * Bump statrs from 0.17.1 to 0.18.0 (#3426) * Bump roaring from 0.10.7 to 0.10.8 (#3423) * Bump needletail from 0.6.0 to 0.6.1 (#3427) * Bump web-sys from 0.3.72 to 0.3.74 (#3411) * Bump js-sys from 0.3.72 to 0.3.74 (#3412) * Bump roaring from 0.10.6 to 0.10.7 (#3413) * Bump serde_json from 1.0.132 to 1.0.133 (#3402) * Bump serde from 1.0.214 to 1.0.215 (#3403)

Release issue: #3481 ---- NOTE: This release adds basic support for skipmers, but they are not yet fully supported. Minor new features: * add genbank plant db to docs (#3429) * add skipmer capacity to sourmash python layer via ffi (#3446) * add skipmers; switch to reading frame approach for translation, skipmers (#3395) * additional moltype specification needed for `sig downsample` with skipmers (#3457) * update with misc animal genomes (#3422) Cleanup and documentation updates: * add comment about semver and column headings (#3433) Developer updates: * Deps: update to rocksdb 0.23 (#3456) * Refactor: Use to_writer/from_reader across the codebase (#3443) * adjust `Signature::name()` to return `Option<String>` instead of `filename()` and `md5sum()` (#3434) * bump version to 4.8.13-dev (#3474) * fix comment in _set_num_scaled (#3451) * propagate zipfile errors (#3431) * update rust CHANGELOG in preparation for r0.18.0 (#3450) * CI: github actions updates (#3476) Dependabot updates: * Bump itertools from 0.13.0 to 0.14.0 (#3471) * Bump needletail from 0.6.0 to 0.6.1 (#3427) * Bump proptest from 1.5.0 to 1.6.0 (#3437) * Bump roaring from 0.10.7 to 0.10.8 (#3423) * Bump roaring from 0.10.8 to 0.10.9 (#3438) * Bump serde from 1.0.215 to 1.0.216 (#3436) * Bump serde from 1.0.216 to 1.0.217 (#3464) * Bump serde_json from 1.0.133 to 1.0.134 (#3453) * Bump statrs from 0.17.1 to 0.18.0 (#3426) * Bump tempfile from 3.14.0 to 3.15.0 (#3472) * Bump thiserror from 2.0.3 to 2.0.6 (#3425) * Bump thiserror from 2.0.6 to 2.0.7 (#3435) * Bump thiserror from 2.0.7 to 2.0.8 (#3448) * Bump thiserror from 2.0.8 to 2.0.9 (#3452) * Update maturin requirement from <1.8.0,>=1 to >=1,<1.9.0 (#3465) * [pre-commit.ci] pre-commit autoupdate (#3428) * [pre-commit.ci] pre-commit autoupdate (#3439) * [pre-commit.ci] pre-commit autoupdate (#3454) * [pre-commit.ci] pre-commit autoupdate (#3473)

ctb added 7 commits December 15, 2024 06:27

WIP: fix intersect_manifest on empty names

2e63b1b

fix compile

5cf7c72

more debug

f1b889c

even more debug

3cc41a1

avoid defaulting name() to filename()

1dccd2c

default to empty string

f5974ef

isolate changes

8a9d657

ctb mentioned this pull request Dec 15, 2024

sourmash scripts singlesketch output differs from sourmash sketch sourmash-bio/sourmash_plugin_branchwater#538

Closed

ctb mentioned this pull request Dec 17, 2024

Signature::name() returns filename() and then md5sum() if .name is empty. #3441

Closed

ctb added 4 commits December 16, 2024 16:15

add test

c84565c

fix name

11db611

fix test

d2300b7

Merge branch 'latest' of github.com:sourmash-bio/sourmash into fix_in…

c0165bd

…tersect_manifest

ctb commented Dec 17, 2024

View reviewed changes

ctb changed the title ~~WIP: make Signature::name() return empty string if name is None~~ MRG: make Signature::name() return empty string if name is None Dec 17, 2024

Merge branch 'latest' into fix_intersect_manifest

0f006e1

luizirber reviewed Dec 17, 2024

View reviewed changes

ctb added 3 commits December 17, 2024 11:50

change Signature::name() to Option<String>

34dba54

switch to using name_str

6ad32c1

cleanup

0ed80c1

ctb changed the title ~~MRG: make Signature::name() return empty string if name is None~~ MRG: adjust Signature::name() to return Option<String> instead of filename() and md5sum() Dec 17, 2024

ctb added 3 commits December 17, 2024 12:12

cargo fmt

2c7a070

fix clippy

e910e7c

wow, ok

6b6df75

luizirber self-requested a review December 18, 2024 17:35

luizirber approved these changes Dec 18, 2024

View reviewed changes

luizirber reviewed Dec 18, 2024

View reviewed changes

src/core/src/signature.rs Outdated Show resolved Hide resolved

update to docstring

9f0702d

ctb enabled auto-merge (squash) December 18, 2024 17:51

ctb disabled auto-merge December 18, 2024 18:20

ctb merged commit f4f5187 into latest Dec 18, 2024
42 of 44 checks passed

ctb deleted the fix_intersect_manifest branch December 18, 2024 18:20

ctb mentioned this pull request Dec 20, 2024

MRG: update rust CHANGELOG in preparation for r0.18.0 #3450

Merged

This was referenced Dec 21, 2024

intersect_manifest fails to match identical Records because of Signature::name() behavior sourmash-bio/sourmash_plugin_branchwater#550

Closed

MRG: update to (prospective) sourmash r0.18.0 sourmash-bio/sourmash_plugin_branchwater#548

Merged

ctb mentioned this pull request Jan 11, 2025

MRG: 4.8.13 release branch #3482

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MRG: adjust `Signature::name()` to return `Option<String>` instead of `filename()` and `md5sum()` #3434

MRG: adjust `Signature::name()` to return `Option<String>` instead of `filename()` and `md5sum()` #3434

ctb commented Dec 15, 2024 •

edited

Loading

codecov bot commented Dec 15, 2024 •

edited

Loading

ctb Dec 17, 2024

ctb Dec 17, 2024

luizirber Dec 17, 2024 •

edited

Loading

ctb Dec 17, 2024

ctb Dec 17, 2024

ctb commented Dec 17, 2024

luizirber Dec 17, 2024

ctb Dec 17, 2024

ctb Dec 17, 2024

ctb commented Dec 17, 2024

		@@ -448,10 +448,8 @@ impl Signature {
		pub fn name(&self) -> String {

MRG: adjust Signature::name() to return Option<String> instead of filename() and md5sum() #3434

MRG: adjust Signature::name() to return Option<String> instead of filename() and md5sum() #3434

Conversation

ctb commented Dec 15, 2024 • edited Loading

codecov bot commented Dec 15, 2024 • edited Loading

Codecov Report

ctb Dec 17, 2024

Choose a reason for hiding this comment

ctb Dec 17, 2024

Choose a reason for hiding this comment

luizirber Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

ctb Dec 17, 2024

Choose a reason for hiding this comment

ctb Dec 17, 2024

Choose a reason for hiding this comment

ctb commented Dec 17, 2024

luizirber Dec 17, 2024

Choose a reason for hiding this comment

ctb Dec 17, 2024

Choose a reason for hiding this comment

ctb Dec 17, 2024

Choose a reason for hiding this comment

ctb commented Dec 17, 2024

MRG: adjust `Signature::name()` to return `Option<String>` instead of `filename()` and `md5sum()` #3434

MRG: adjust `Signature::name()` to return `Option<String>` instead of `filename()` and `md5sum()` #3434

ctb commented Dec 15, 2024 •

edited

Loading

codecov bot commented Dec 15, 2024 •

edited

Loading

luizirber Dec 17, 2024 •

edited

Loading