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

Documentation about the PR reviewing process #1035

Closed
maxulysse opened this issue Feb 28, 2022 · 6 comments · Fixed by #1710
Closed

Documentation about the PR reviewing process #1035

maxulysse opened this issue Feb 28, 2022 · 6 comments · Fixed by #1710

Comments

@maxulysse
Copy link
Member

Following a discussion with @sateeshperi we figured it would be a good idea to have some documentation/guidelines about how to actually do a review (I'm thinking also with some specifics about modules, pipelines, tools, website, release...)
It would be super helpful for newcomers.

@maxulysse maxulysse added enhancement New feature or request documentation labels Feb 28, 2022
@maxulysse
Copy link
Member Author

@mahesh-panchal probably gave some good insight on this idea as well as usual :-D

@sateeshperi
Copy link
Contributor

leaving points here...to be structured later...plz add as you see fit @maxulysse @mahesh-panchal @FriederikeHanssen

when all checks pass

  • all optional params must go to $args
  • must include tests for all output including optional
  • optimize the software version extraction command if required
  • check meta.yml for documentation links; patterns of files;
  • check if the bioconda version of tool is latest ?

when tests fail

  • linting fails
  • conda fails
  • docker fails

@mahesh-panchal
Copy link
Member

mahesh-panchal commented Mar 1, 2022

  • Run tool help and check important input ( usually optional ) has not been missed.
  • Check module is suitable for offline running, e.g. from secure servers with no Internet.
    • No automatic database downloads assumed.
  • Remove temporary unzipped files ( otherwise benefit is completely mitigated and problem made worse).
  • Check all outputs are captured, by running the pytest (e.g. on Gitpod).

@mahesh-panchal
Copy link
Member

mahesh-panchal commented Mar 1, 2022

@FriederikeHanssen
Copy link
Contributor

@mahesh-panchal
Copy link
Member

Don't know where to put this so I'll just drop it here as a reminder, and may be helpful information for reviewing.
When running docker containers, Nextflow changes the --entrypoint to /bin/bash. Usually for some containers on Biocontainers (and others), the default entrypoint is /usr/local/env-execute, which then sets environment variables used by certain programs (e.g. Busco, Merqury). The files need to be sourced again to use them in container setting ( nf-core/modules#1239 (comment) )

@ewels ewels removed the enhancement New feature or request label Sep 12, 2022
@atrigila atrigila self-assigned this Mar 28, 2023
@atrigila atrigila moved this from Todo to In Progress in nf-core Hackathon March 2023 Mar 28, 2023
@atrigila atrigila linked a pull request Mar 28, 2023 that will close this issue
@atrigila atrigila moved this from In Progress to Ready for review in nf-core Hackathon March 2023 Mar 28, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in Hackathon-March-2022 Mar 28, 2023
@github-project-automation github-project-automation bot moved this from Ready for review to Done in nf-core Hackathon March 2023 Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants