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

Force sig file extension to be sig or sig.gz #1932

Open
mr-eyes opened this issue Apr 6, 2022 · 4 comments
Open

Force sig file extension to be sig or sig.gz #1932

mr-eyes opened this issue Apr 6, 2022 · 4 comments
Labels
5.0 issues to address for a 5.0 release
Milestone

Comments

@mr-eyes
Copy link
Member

mr-eyes commented Apr 6, 2022

Since the user has the power to sig rename or generates signatures without .sig or .sig.gz extension. And since the zipStorage implementation expects .sig or .sig.gz extension as per

if filename.endswith('.sig') or \
filename.endswith('.sig.gz') or \

Enforcing/unifying signature extensions to be .sig or .sig.gz can reduce some errors.

@mr-eyes
Copy link
Member Author

mr-eyes commented Apr 6, 2022

Maybe I am getting it wrong? due to the traverse_yield_all after that?
@luizirber @ctb

@ctb
Copy link
Contributor

ctb commented Apr 6, 2022

Hi @mr-eyes I don't understand -

All of the internal code reads and writes .sig/.sig.gz file names by default and by convention. The user can specify a different file extension if they want and it will be read appropriately, but only .sig and .sig.gz files will be auto-discovered on traversal unless -f is specified.

So, for example, if a directory has a bunch of .sig.gz files under it, and you point sourmash at the directory it will load all those files and no more, unless you specify -f. Same with a zip file.

@ctb
Copy link
Contributor

ctb commented Apr 6, 2022

oh! perhaps you mean we should require it all the time?

I don't recall why I thought it was a good idea Back When, and it causes some interesting problems and weird behavior (see comments about 'traverse_all' in #1849). But it isn't our convention and I don't think it causes too many problems in the code. I don't have any real problem with changing it but it would have to be part of a major version bump (at least 1, if not 2) because of semantic versioning. I'm tempted to say that we should just leave it.

@mr-eyes mr-eyes added the 5.0 issues to address for a 5.0 release label Apr 6, 2022
@mr-eyes
Copy link
Member Author

mr-eyes commented Apr 6, 2022

oh! perhaps you mean we should require it all the time?

Yes exactly!

I don't have any real problem with changing it but it would have to be part of a major version bump (at least 1, if not 2) because of semantic versioning. I'm tempted to say that we should just leave it.

Ok then, I labeled it to the next major version for now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 issues to address for a 5.0 release
Projects
None yet
Development

No branches or pull requests

3 participants