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

Merging template updates and building schema #88

Merged
merged 20 commits into from
Aug 13, 2020

Conversation

skrakau
Copy link
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
Copy link
Member

@ggabernet ggabernet left a comment

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 👍

@ggabernet ggabernet self-requested a review August 12, 2020 17:01
@ggabernet
Copy link
Member

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

@ggabernet
Copy link
Member

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

@skrakau
Copy link
Member Author

skrakau commented Aug 12, 2020

Yes, sorry, I missed that. Will update

@skrakau
Copy link
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
Member

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
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
Member

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 😄

Co-authored-by: Gisela Gabernet <[email protected]>
@skrakau
Copy link
Member Author

skrakau commented Aug 12, 2020

Thank you @ggabernet :)

@ggabernet
Copy link
Member

sure! 😄

@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants