Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ctb opened this issue Dec 17, 2024 · 1 comment · Fixed by #3434
Closed

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

ctb opened this issue Dec 17, 2024 · 1 comment · Fixed by #3434

Comments

@ctb
Copy link
Contributor

ctb commented Dec 17, 2024

When Signature::name is None, the accessor method returns filename() and then md5sum() instead of an empty string, "".

This causes problems because (e.g.) Record::from_sig uses name() to construct the Record, but then this causes PartialEq to fail to match records that have empty names and are built elsewhere. This is what is causing intersect_manifest to fail over in sourmash-bio/sourmash_plugin_branchwater#538 with standalone manifests built with empty names.

In #3434, I propose having an empty name return empty string instead.

Alternatives would be to provide a different accessor function (raw_name()?) or has_name() or something. But since no tests seem to depend on the current behavior, and nothing breaks, I think it's ok to change.

(The inspiration from this behavior was from the Python layer, where we provide a display name.)

@ctb
Copy link
Contributor Author

ctb commented Dec 17, 2024

ooh, I was wrong ;). one of the tests fails! will fix.

@ctb ctb closed this as completed in #3434 Dec 18, 2024
ctb added a commit that referenced this issue Dec 18, 2024
… `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 #3441
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant