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

migration to DSL2 #104

Merged
merged 66 commits into from
Nov 8, 2021
Merged

migration to DSL2 #104

merged 66 commits into from
Nov 8, 2021

Conversation

lpantano
Copy link
Contributor

@lpantano lpantano commented Sep 9, 2021

PR checklist

When merging, the test config files need to point to the main branch of the datatest repository and for that this PR needs to be merged: nf-core/test-datasets#363

Note that the versions are still in TXT format because I wasn't sure on how to get it working with YML but maybe that could be another PR if it is the only thing...

  • 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 you've added a new tool - add to the software_versions process and a regex to scrape_software_versions.py
    • If you've added a new tool - have you followed the pipeline conventions in the [contribution docs](https://github.com/nf-core/smrnaseq/tree/master/.github/CONTRIBUTING.md)
    • If necessary, also make a PR on the nf-core/smrnaseq branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint .).
  • Ensure the test suite passes (nextflow run . -profile test,docker).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@github-actions
Copy link

github-actions bot commented Sep 9, 2021

Markdown linting is failing

To keep the code consistent with lots of contributors, we run automated code consistency checks.
To fix this CI test, please run:

  • Install markdownlint-cli
  • Fix the markdown errors
    • Automatically: markdownlint . --fix
    • Manually resolve anything left from markdownlint .

Once you push these changes the test should pass, and you can hide this comment 👍

We highly recommend setting up markdownlint in your code editor so that this formatting is done automatically on save. Ask about it on Slack for help!

Thanks again for your contribution!

@github-actions
Copy link

github-actions bot commented Sep 9, 2021

YAML linting is failing

To keep the code consistent with lots of contributors, we run automated code consistency checks.
To fix this CI test, please run:

  • Install yaml-lint
  • Fix the markdown errors
    • Run the test locally: yamllint $(find . -type f -name "*.yml" -o -name "*.yaml")
    • Fix any reported errors in your YAML files

Once you push these changes the test should pass, and you can hide this comment 👍

We highly recommend setting up yaml-lint in your code editor so that this formatting is done automatically on save. Ask about it on Slack for help!

Thanks again for your contribution!

lpantano and others added 8 commits October 21, 2021 11:13
Co-authored-by: Kevin <klkeys@users.noreply.github.com>
Co-authored-by: Kevin <klkeys@users.noreply.github.com>
Co-authored-by: Kevin <klkeys@users.noreply.github.com>
Co-authored-by: Kevin <klkeys@users.noreply.github.com>
Co-authored-by: Kevin <klkeys@users.noreply.github.com>
Co-authored-by: Kevin <klkeys@users.noreply.github.com>
Co-authored-by: Kevin <klkeys@users.noreply.github.com>
Co-authored-by: Kevin <klkeys@users.noreply.github.com>
@klkeys klkeys self-requested a review November 1, 2021 16:58
Copy link
Member

@JoseEspinosa JoseEspinosa left a comment

Choose a reason for hiding this comment

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

Huge work, very nice! 😄
Most of my comments are related to the new way of reporting versions, although there are some others related to more general stuff.
Also, I was wondering whether some of the local modules (e.g. bowtie/build and bowtie/align) could not be used from nf-core modules instead, but might be that I am not aware of all the details and that it is not possible to use them in this pipeline.
Almost there 🚀

.github/workflows/ci.yml Show resolved Hide resolved
.nf-core.yml Show resolved Hide resolved
CITATIONS.md Show resolved Hide resolved
bin/scrape_software_versions.py Show resolved Hide resolved
subworkflows/local/bam_sort.nf Outdated Show resolved Hide resolved
workflows/smrnaseq.nf Outdated Show resolved Hide resolved
workflows/smrnaseq.nf Outdated Show resolved Hide resolved
workflows/smrnaseq.nf Show resolved Hide resolved
nextflow.config Outdated Show resolved Hide resolved
nextflow.config Outdated Show resolved Hide resolved
Copy link
Contributor

@FriederikeHanssen FriederikeHanssen left a comment

Choose a reason for hiding this comment

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

Great work!! 🚀 Just a couple of minor things, mostly regarding versioning (which I see now also Jose has reviewed already). I think adding the citations would be fairly important too.

CITATIONS.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
bin/scrape_software_versions.py Show resolved Hide resolved
modules/local/seqcluster_collapse.nf Outdated Show resolved Hide resolved
modules/local/trimgalore.nf Show resolved Hide resolved
modules/nf-core/modules/samtools/view/main.nf Outdated Show resolved Hide resolved
params.samtools_stats_options = [:]
params.table_merge_options = [:]

include { PARSE_FASTA_MIRNA as PARSE_MATURE
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe align for readability

subworkflows/local/mirna_quant.nf Show resolved Hide resolved
@apeltzer
Copy link
Member

apeltzer commented Nov 5, 2021

I will wait until Joses / Frederikes requests are accepted / discussed & then have a look afterwards - please ping me then here directly or via Slack 👍🏻

lpantano and others added 7 commits November 5, 2021 09:25
Co-authored-by: Jose Espinosa-Carrasco <kadomu@gmail.com>
Co-authored-by: Jose Espinosa-Carrasco <kadomu@gmail.com>
Co-authored-by: FriederikeHanssen <Friederike.hanssen@qbic.uni-tuebingen.de>
Co-authored-by: FriederikeHanssen <Friederike.hanssen@qbic.uni-tuebingen.de>
Co-authored-by: Jose Espinosa-Carrasco <kadomu@gmail.com>
Co-authored-by: FriederikeHanssen <Friederike.hanssen@qbic.uni-tuebingen.de>
@lpantano
Copy link
Contributor Author

lpantano commented Nov 5, 2021

Thanks @apeltzer.
@JoseEspinosa @FriederikeHanssen I tried to adapt all the comments not related to version.

Some modules are local even if nf-core has similar modules because there is something unique to smrnaseq and it would take some evaluation to do the adaptation.

I am leaving the versioning, so @JoseEspinosa can make the first change to use the YAML format, and I can then modify all the local modules to that format as well.

The plan was to merge, and the @JoseEspinosa will make another PR with the versioning and I will work on top of that. I hope that is ok for everybody else.

As well, there is a final module to add, but I need to do the docker containers, so I am skipping it here. (EDGER module in mirna_quant.nf)

Thanks!

Copy link
Member

@JoseEspinosa JoseEspinosa left a comment

Choose a reason for hiding this comment

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

I think most of the changes have been addressed apart from the implementation of the new versioning. Thanks @lpantano ! 🚀
I will merge this PR and work on the versioning on top of them as we agreed.
Also, I will close #101 since we are addressing the merging of the new nf-core template on this PR.

Copy link
Contributor

@FriederikeHanssen FriederikeHanssen left a comment

Choose a reason for hiding this comment

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

Looks good :) Great work 🥳 , and as discussed remaining things can go on top of the versioning PR

Comment on lines +2 to +5
include { saveFiles; initOptions; getSoftwareName } from './functions'

params.options = [:]
options = initOptions(params.options)
Copy link
Contributor

Choose a reason for hiding this comment

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

Commit this?

@JoseEspinosa JoseEspinosa merged commit 0444a9c into dev Nov 8, 2021
@apeltzer apeltzer deleted the dsl2 branch January 3, 2022 12:41
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.

7 participants