-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add validation for fasta input and tests #20
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good, hopefully it is tested as I have very little knowledge of nf-schema. Should we not update the readme and usage to reflect the changes in the input format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just some very minor comments
docs/usage.md
Outdated
| ------- | ------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| `id` | User-defined identifier. It is used to identify output files for the protein. Can be anything descriptive, as long as it does not contain spaces. | | ||
| `query` | The query of the user-specified type. It should be a valid Uniprot accession. | | ||
| `fasta` | It should be a valid path to a FASTA file. | | ||
|
||
An [example samplesheet](../assets/samplesheet.csv) has been provided with the pipeline. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want an example for FASTA samplesheet as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is changed in line 33, or are you referring to something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 43 mentions a separate samplesheet file, I wonder if we need a similar one for FASTA. Just a question to consider, not necessarily demanding it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 195ab8d
Co-authored-by: Igor Trujnara <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final tiny tweak
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fasta
input.samplesheet
intosamplesheet_query
andsamplesheet_fasta
and refactor code accordingly.fasta
, run with-profile test_fasta
.test_fasta
on CI.Closes #17
PR checklist
nf-core lint
).nf-test test main.nf.test -profile test,docker
).docs/usage.md
is updated.README.md
is updated (including new tool citations and authors/contributors).