-
Notifications
You must be signed in to change notification settings - Fork 212
Include metagenomics mini-course #577
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
base: master
Are you sure you want to change the base?
Conversation
Hi @jeffe107, thanks for this! Sorry for the delay, I've had a lot on my plate. I've now started reviewing your PR (finally) and my initial impression is that this is in line with what we discussed, the structure follows our guidelines etc so at the top level that's all really good. Next I'm going to review the content in more detail to familiarize myself better with the actual analysis that is presented, and review from an educational standpoint. I'll comment here if anything comes up as I work my way through this. |
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.
Thanks for contributing this detailed metagenomics pipeline tutorial!
- Prerequisites changes requested: This tutorial uses a few points of syntax that are not covered in Hello Nextflow, so they should be documented explicitly. One is the use of a main.nf and workflow with take/main syntax; for that you can point to the "Workflows of workflows" Side Quest. The other that jumped out at me is the use of
log.info
for which it would be good to link to documentation. - Formatting changes requested: We prefer to start all sentences on a new line, even within paragraphs; see https://github.com/nextflow-io/training/pull/612/files (I realized this wasn't documented yet, so I've added a blurb to the CONTRIBUTION.md doc).
- I have a few edit suggestions for language but it would be best to do that after you update the text to have all sentences start on new lines. This will make tracking changes much easier.
- I think we should add a
!!! note
box in the course description acknowledging that this is a community contribution, both to credit you appropriately as the author, and to set expectations regarding support and maintenance. - At some point in the near future we're going to rework how the left-hand menu displays courses, because we need to group/collapse the 'science' courses together for readability. But I don't want to hold up this PR on that, we can use the current setup and do that once this is merged.
The two I've bolded are top priority in my opinion. For the "community contribution" box we need to think a little about what the language should be, but feel free to propose something based on how you'd like to be credited and we can edit from there.
Hello, Thank you for your comments. I already applied the main changes that you suggested in this PR. I included the !!!note at the very beggining of the course, please feel free to modify it and move it to he best location you consider suitable. I remain ready to apply further changes you consider necessary. Best, |
Thanks @jeffe107! I'm finishing up a couple of big chunks of work then I'll work on getting this over the finish line. |
This pull request includes both the pipeline and the documentation.