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

Last component of output base names containing multiple periods is dropped #907

Closed
KatSteinke opened this issue Jun 29, 2022 · 4 comments · Fixed by #913
Closed

Last component of output base names containing multiple periods is dropped #907

KatSteinke opened this issue Jun 29, 2022 · 4 comments · Fixed by #913
Labels
prio:high Do this immediately t:bug Type: bug, error, something isn't working

Comments

@KatSteinke
Copy link

I've run into an issue when specifying an output-basename that contains multiple periods such as e.g. SAMPLENAME.very.long.basename in Nextclade 2.0.0 - the last component of the name is consistently dropped, so result files are named e.g. SAMPLENAME.very.long.[result].[extension] .
For more detail: I'm running this with output-all, output-basename, output-translations and output-csv specified; the latter two are absolute paths, and work without problems.

@KatSteinke KatSteinke added good first issue Good for newcomers help wanted Extra attention is needed needs triage Mark for review and label assignment t:bug Type: bug, error, something isn't working labels Jun 29, 2022
@corneliusroemer corneliusroemer removed good first issue Good for newcomers help wanted Extra attention is needed labels Jun 29, 2022
@corneliusroemer
Copy link
Member

corneliusroemer commented Jun 29, 2022

Thanks for reporting. I can reproduce this:

nextclade dataset get --name='sars-cov-2' --output-dir=data
nextclade run -D data --output-all out --output-basename basename.THISWILLDISAPPEAR -t out/test.tsv data/sequences.fasta --output-selection fasta
ls out

The result is:

-rw-r--r--  1 cr staff 3.1M Jun 29 14:21 basename.aligned.fasta

As you report, .THISWILLDISAPPEAR has disappeared.

@corneliusroemer corneliusroemer added the prio:high Do this immediately label Jun 29, 2022
@corneliusroemer
Copy link
Member

@KatSteinke for now a workaround would be to add an extra ending that will get stripped. I'll try to get a fix out today @ivan-aksamentov

@corneliusroemer
Copy link
Member

corneliusroemer commented Jun 29, 2022

Root cause:

output_fasta.get_or_insert(default_output_file_path.with_extension("aligned.fasta"));

with_extension() replaces the extension if present. There's no stdlib way to add_extension yet unfortunately.

ivan-aksamentov added a commit that referenced this issue Jun 29, 2022
Resolves: #907

This rolls an in-house version of `add_extension()` function which always adds an extension to a `PathBuf`.  This is different from `PathBuf::with_extension()` which may replace or add extension depending on what the path is.

This solves a problem with basenames containing dots, as described in the issue: `PathBuf::with_extension()` thought that they are extensions and replaced the last one. But we always want to add, not replace.
@ivan-aksamentov ivan-aksamentov removed the needs triage Mark for review and label assignment label Jun 29, 2022
@ivan-aksamentov
Copy link
Member

This was fixed in #913 and will be released in 2.0.1 in a few moments.

corneliusroemer added a commit that referenced this issue Jan 23, 2024
Reasoning in https://github.com/nextstrain/nextclade/pull/1387/files#r1462588942

I tested with test case from #907 and various basenames and outputs are sensible. This refactor doesn't seem to change anything - at least I couldn't find a difference but it's good to be safe.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio:high Do this immediately t:bug Type: bug, error, something isn't working
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants