Skip to content

Merging template updates and building schema#88

Merged
skrakau merged 20 commits intonf-core:devfrom
skrakau:merging-template-updates
Aug 13, 2020
Merged

Merging template updates and building schema#88
skrakau merged 20 commits intonf-core:devfrom
skrakau:merging-template-updates

Conversation

@skrakau
Copy link
Copy Markdown
Member

@skrakau skrakau commented Aug 12, 2020

Here are the updates for the new nf-core template as well as the changes needed for the new json schema.

I changed the paths in output.md, since the sentence "All paths are relative to the top-level results directory." was added to the template.

Currently there are no full size tests yet, thus the standard test profile is still specified in .github/workflows/awsfulltest.yml (created an issue for this #87).

And I removed the pipeline specific parameters from the "usage.md", since this will be included with the json schema. The help message I didn't touch, since there were no changes in the template for this. I guess the functionality that this can be replaced with the schema will come with future templates or DSL2.

Let me know if I forgot something.

PR checklist

  • This comment contains a description of changes (with reason)
  • If you've fixed a bug or added code that should be tested, add tests!
  • If necessary, also make a PR on the nf-core/mag branch on the nf-core/test-datasets repo
  • Ensure the test suite passes (nextflow run . -profile test,docker).
  • Make sure your code lints (nf-core lint .).
  • Documentation in docs is updated
  • CHANGELOG.md is updated
  • README.md is updated

Learn more about contributing: https://github.com/nf-core/mag/tree/master/.github/CONTRIBUTING.md

@skrakau skrakau requested review from d4straub and ggabernet August 12, 2020 16:10
@skrakau skrakau force-pushed the merging-template-updates branch from 05cd579 to ddbec8d Compare August 12, 2020 16:51
@skrakau skrakau force-pushed the merging-template-updates branch from ddbec8d to b52035c Compare August 12, 2020 16:57
ggabernet
ggabernet approved these changes Aug 12, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@ggabernet ggabernet left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me, after the small corrections 👍

Comment thread .github/workflows/awstest.yml Outdated
Comment thread docs/output.md Outdated
Comment thread main.nf Outdated
@ggabernet ggabernet self-requested a review August 12, 2020 17:01
@ggabernet
Copy link
Copy Markdown
Member

ggabernet commented Aug 12, 2020

Ah wait, linting is failing, there are some markdown issues in output.md and readme.md

@ggabernet
Copy link
Copy Markdown
Member

ggabernet commented Aug 12, 2020

And I think that nf-core lint is failing cause the input_paths parameter is missing in the json schema

@skrakau
Copy link
Copy Markdown
Member Author

skrakau commented Aug 12, 2020

Yes, sorry, I missed that. Will update

@skrakau
Copy link
Copy Markdown
Member Author

skrakau commented Aug 12, 2020

Although nf-core schema lint . says it's valid (didn't want that parameter there, but then I will add it and hide).

@ggabernet
Copy link
Copy Markdown
Member

ggabernet commented Aug 12, 2020

yes, that seems like a good solution, to add it but keep it hidden. I can understand that you did not want it there

@skrakau
Copy link
Copy Markdown
Member Author

skrakau commented Aug 12, 2020

OK, I checked the template again, and it seems this parameter doesn't necessarily need to be in nextflow.config, and then maybe it also doesn't need to show up in the schema? Would wait for the test here, but my local linting doesn't fail

@ggabernet
Copy link
Copy Markdown
Member

ggabernet commented Aug 12, 2020

yes, that should be fine, it was created to use only in the test profile so read paths can be provided there. Don't forget to accept my suggestions though if you agree to them, as they solve a couple of bugs 😄

@skrakau @ggabernet
Update docs/output.md
ce598d4 
Co-authored-by: Gisela Gabernet <[email protected]>
@skrakau
Copy link
Copy Markdown
Member Author

skrakau commented Aug 12, 2020

Thank you @ggabernet :)

@ggabernet
Copy link
Copy Markdown
Member

ggabernet commented Aug 12, 2020

sure! 😄

d4straub
d4straub approved these changes Aug 13, 2020
View reviewed changes
Comment thread .github/workflows/ci.yml Outdated
Comment thread conf/igenomes.config
Comment thread docs/output.md Outdated
Comment thread docs/usage.md
@skrakau skrakau merged commit b9aa53e into nf-core:dev Aug 13, 2020
@skrakau skrakau deleted the merging-template-updates branch May 31, 2021 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@d4straub d4straub d4straub approved these changes

@ggabernet ggabernet Awaiting requested review from ggabernet

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@skrakau @ggabernet @d4straub @nf-core-bot