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

PSEUDO PR - DO NOT MERGE #2

Closed
wants to merge 26 commits into from
Closed

PSEUDO PR - DO NOT MERGE #2

wants to merge 26 commits into from

Conversation

ewels
Copy link
Member

@ewels ewels commented Jun 17, 2022

PSEUDO PR - DO NOT MERGE

This is the pseudo-PR that we use for the community review leading up to the first release of this nf-core pipeline.

Please do not merge the PR, as that closes it and all associated discussion. However, you can use the GitHub reviewing interface to add reviews and inline comments to the code.


To do before first release

  1. At least one more review from someone else, preferably two
  2. Check documentation is up to date and complete
  3. Test running the pipeline offline
  4. Remove nf-core lint warnings if possible

Update Markdown

All Markdown files containing information about the pipeline need to be up to date including output.md, usage.md and README.md.

  1. README.md
  2. output.md
  3. usage.md
  4. nextflow_schema.json (documentation via nf-core schema build)

@ewels ewels added the WIP Work in progress label Jun 17, 2022
@github-actions
Copy link

github-actions bot commented Jun 17, 2022

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 29e6cbc

+| ✅ 146 tests passed       |+
!| ❗   3 tests had warnings |!

❗ Test warnings:

  • nextflow_config - Config manifest.version should end in dev: '1.0.0'
  • readme - README did not have a Nextflow minimum version badge.
  • pipeline_todos - TODO string in WorkflowMain.groovy: Add Zenodo DOI for pipeline after first release

✅ Tests passed:

Run details

  • nf-core/tools version 2.4.1
  • Run at 2022-06-20 11:24:11

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.

Awesome job @sguizard! 🚀
I just dropped some suggestions but otherwise looks almost ready for release. It might be worth thought that someone familiar with isoseq analysis might take a look too.

CITATIONS.md Show resolved Hide resolved
CITATIONS.md 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
modules/local/gstama/filelist/main.nf Outdated Show resolved Hide resolved
modules/nf-core/modules/bamtools/convert/main.nf Outdated Show resolved Hide resolved
nextflow.config Show resolved Hide resolved
nextflow_schema.json Outdated Show resolved Hide resolved
workflows/isoseq.nf Show resolved Hide resolved
sguizard and others added 15 commits June 17, 2022 09:45
Co-authored-by: Jose Espinosa-Carrasco <[email protected]>
Co-authored-by: Jose Espinosa-Carrasco <[email protected]>
Co-authored-by: Jose Espinosa-Carrasco <[email protected]>
Co-authored-by: Jose Espinosa-Carrasco <[email protected]>
Co-authored-by: Jose Espinosa-Carrasco <[email protected]>
Remove stageInMode
Replace biocontainers/biocontainers:v1.2.0_cv1 by ubuntu:20.04
Correct publish mode setup
 Set default to null
Reintegrate CUSTOM_DUMPSOFTWAREVERSIONS
uncomment SAMPLESHEET_CHECK and CUSTOM_DUMPSOFTWAREVERSIONS configs
Co-authored-by: Jose Espinosa-Carrasco <[email protected]>
@sguizard
Copy link
Collaborator

@JoseEspinosa I applied your corrections/suggestions.
Can you give assistance to clean the test-datasets master branch? I first pushed the data on it 😅

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.

Thanks a lot @sguizard 🚀

CITATIONS.md Show resolved Hide resolved
@JoseEspinosa
Copy link
Member

@JoseEspinosa I applied your corrections/suggestions. Can you give assistance to clean the test-datasets master branch? I first pushed the data on it 😅

Sure, you just need to create a PR to the master branch in which you remove the files you included there, ping me and I can review it.

Copy link

@DSchreyer DSchreyer left a comment

Choose a reason for hiding this comment

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

The pipeline is in a good state for publication. I just had some minor comments, which can be resolved easily. The pipeline works as expected and the documentation is almost complete. Imo it is ready for the official review.

bin/check_samplesheet.py Outdated Show resolved Hide resolved
nextflow_schema.json Show resolved Hide resolved
@sguizard
Copy link
Collaborator

@JoseEspinosa @DSchreyer I finished making corrections, Thanks for your reviews!
What is the next step now?

@DSchreyer
Copy link

@JoseEspinosa @DSchreyer I finished making corrections, Thanks for your reviews! What is the next step now?

@DSchreyer DSchreyer closed this Jun 20, 2022
@DSchreyer DSchreyer reopened this Jun 20, 2022
@sguizard
Copy link
Collaborator

Update of MultiQC broke the pipeline, let me fix this please

@DSchreyer
Copy link

e, let me fix this please

Yes, sorry. closing issue was unintentionally. Could you also address the last review of the css parameters in the nextflow_schema.json file. Then you are good to go!

@sguizard
Copy link
Collaborator

All Fixed!

@DSchreyer
Copy link

Well done @sguizard ! Nothing to add !

@JoseEspinosa
Copy link
Member

@JoseEspinosa @DSchreyer I finished making corrections, Thanks for your reviews! What is the next step now?
Sorry for the late reply. Now you can go for your first release 🚀 See here. There is a release checklist on this link and if you have any doubts just drop them on slack.

@sguizard sguizard closed this Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants