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

Adapt code and docs to nf-core template #15

Merged
merged 24 commits into from May 31, 2019

Conversation

lpantano
Copy link
Contributor

This PR adapt the code and docs to the latest nf-core template.

It is a mixed approach between manual copy/paste and git merge. I will need somebody to look into the most important thing I cannot miss since I am not sure if I covered all.

Thanks!

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!
  • 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/smrnaseq/tree/master/.github/CONTRIBUTING.md

@drpatelh
Copy link
Member

This looks great @lpantano ! I've spotted a couple of things that may need to be changed but I'm looking on my mobile which isn't the greatest...Can have a look next week or before if I get some time.

You should be able to correct the markdownlint errors in the docs:
https://github.com/DavidAnson/markdownlint

Should the pipeline running end-to-end with the test dataset? If not, it would be great if you could do that before we merge this PR.

It looks like the test dataset failed because the gtf file couldn't be found. It does exist though so Travis may need to be restarted. I've been seeing that quite a bit over the last couple of days.

@lpantano
Copy link
Contributor Author

lpantano commented May 25, 2019 via email

Copy link
Member

@drpatelh drpatelh left a comment

Choose a reason for hiding this comment

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

Mainly minor issues @lpatano! Looking good. I still have to have a look at main.nf properly but Ive had a look at the rest of the files.

assets/email_template.html Outdated Show resolved Hide resolved
assets/multiqc_config.yaml Outdated Show resolved Hide resolved
bin/scrape_software_versions.py Show resolved Hide resolved
conf/base.config Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
nextflow.config Outdated Show resolved Hide resolved
nextflow.config Outdated Show resolved Hide resolved
main.nf Show resolved Hide resolved
Copy link
Member

@apeltzer apeltzer left a comment

Choose a reason for hiding this comment

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

Some smallish things , otherwise 100% in line with @drpatelh 's comments - go Lorena :-) 👍

assets/email_template.html Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@lpantano
Copy link
Contributor Author

Thanks so much for looking into this @drpatelh, @apeltzer. I will work on this in the next two days and trying not to miss any of the changes you propose.

Copy link
Member

@drpatelh drpatelh left a comment

Choose a reason for hiding this comment

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

Finished going through main.nf 👍

main.nf Outdated Show resolved Hide resolved
@lpantano
Copy link
Contributor Author

ok, I think I fixed all the comments, but maybe I misunderstood any, if you could check it would be great. I have two main questions:

  • versions of R packages should be included as well in the software version chunk?
  • the multiqc_config.yaml file is correct?

Thanks!

Copy link
Member

@drpatelh drpatelh left a comment

Choose a reason for hiding this comment

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

@lpantano Looking much better! Mostly formatting issues that hopefully shouldnt take too long to resolve 👍

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
conf/base.config Outdated Show resolved Hide resolved
conf/base.config Outdated Show resolved Hide resolved
nextflow.config Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
main.nf Show resolved Hide resolved
main.nf Outdated Show resolved Hide resolved
main.nf Outdated Show resolved Hide resolved
@lpantano
Copy link
Contributor Author

ok, I think I added all the changes. Let me know if you spot something off. As well, not sure what I have to do with the TEMPLATE branch. I am making all my changes in the merge versions, so...can be the TEMPLATE the same than dev as a first version when this is in good shape?

Copy link
Member

@drpatelh drpatelh left a comment

Choose a reason for hiding this comment

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

Almost there @lpantano 😄

Thank you!

main.nf Outdated Show resolved Hide resolved
main.nf Outdated Show resolved Hide resolved
main.nf Outdated Show resolved Hide resolved
lpantano and others added 3 commits May 29, 2019 15:26
Co-Authored-By: Harshil Patel <drpatelh@users.noreply.github.com>
Co-Authored-By: Harshil Patel <drpatelh@users.noreply.github.com>
Co-Authored-By: Harshil Patel <drpatelh@users.noreply.github.com>
main.nf Outdated Show resolved Hide resolved
@drpatelh
Copy link
Member

Great work @lpantano . We finally got there 😃 The template and overall structure looks good to me. I havent looked at the processes much but I guess you are going to develop that more now anyway.

@drpatelh drpatelh dismissed apeltzer’s stale review May 31, 2019 19:06

Review points have been addressed

@drpatelh drpatelh merged commit 97be40b into nf-core:dev May 31, 2019
@lpantano
Copy link
Contributor Author

wooo happy weekend! yes, I will develop from here!

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.

None yet

3 participants