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

[Improvement][WIP] Add subworkflow documentation #55

Merged
merged 16 commits into from
Apr 17, 2024

Conversation

ThoumyreStanislas
Copy link
Contributor

@ThoumyreStanislas ThoumyreStanislas commented Feb 1, 2024

Describe your changes

Say which test data are used by your module

Checklist before requesting a review

  • Create the tool:
    • Edit ./modules/nf-scil/<category>/<tool>/main.nf
    • Edit ./modules/nf-scil/<category>/<tool>/meta.yml
  • Generate the tests:
    • Edit ./tests/modules/nf-scil/<category>/<tool>/main.nf
    • Edit ./tests/modules/nf-scil/<category>/<tool>/nextflow.config
    • Add test data locally for tests with the fork repository
    • Generate the test infrastructure and md5sum for all outputs
  • Ensure the syntax is correct :
    • Check indentation abides with the rest of the library (don't hesitate to correct others !)
    • Lint everything. Ensure your variables have good names.

@ThoumyreStanislas ThoumyreStanislas changed the title add subworkflow documentation [WIP] Add subworkflow documentation Feb 1, 2024
@AlexVCaron AlexVCaron linked an issue Feb 23, 2024 that may be closed by this pull request
@AlexVCaron AlexVCaron changed the title [WIP] Add subworkflow documentation [Improvement][WIP] Add subworkflow documentation Mar 8, 2024
Copy link
Collaborator

@AlexVCaron AlexVCaron left a comment

Choose a reason for hiding this comment

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

Very nice work , all the content seems to be there ! I will try the steps to create a module before Thursday and get back to you. I would however like a slight reshoot over the text. There is a lot of verb tenses that switch throughout, that also make you switch subject a lot.

For example, you start with imperative tense at line 5, then switch at line 22. I would refrain using we altogether, and use only imperative and impersonal statements. It would make the tutorial flow better and look more like a list of steps to follow.

As for the tests documentation, I'll do a shot on that with Anthony on another PR, where he'll document nf-test. It'll go in another documentation file, we need to move some common things from MODULES.md as well. I'll work the PR and get back to you on how we integrate it here.

docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@AlexVCaron AlexVCaron left a comment

Choose a reason for hiding this comment

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

It's going on well. A few comments, then I think it'll be good to go ! We'll merge without the nf-test documentation inserted and add it in another PR, with the help of @gagnonanthony and some changes I have made to the remaining doc.

docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Show resolved Hide resolved
Copy link
Collaborator

@AlexVCaron AlexVCaron left a comment

Choose a reason for hiding this comment

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

Last round of suggestions, then we should done with this one !

docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
@AlexVCaron
Copy link
Collaborator

This is all good for me ! I'll let @gagnonanthony and @Manonedde go through one time and validate with their experience building wokrflows. Then, LGTM !

@AlexVCaron AlexVCaron requested review from Manonedde and gagnonanthony and removed request for arnaudbore April 16, 2024 20:40
Copy link
Contributor

@gagnonanthony gagnonanthony 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! I made minor suggestions, but other than that, very nice! 💯

docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
docs/SUBWORKFLOWS.md Outdated Show resolved Hide resolved
Copy link
Contributor

@gagnonanthony gagnonanthony left a comment

Choose a reason for hiding this comment

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

LGTM ! 🍻

@AlexVCaron AlexVCaron merged commit 06c817e into scilus:main Apr 17, 2024
9 checks passed
@AlexVCaron
Copy link
Collaborator

GJ @ThoumyreStanislas 🥳

@ThoumyreStanislas ThoumyreStanislas deleted the doc_subworkflows branch June 13, 2024 18:53
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.

Create subworkflows documentation
3 participants