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.

Sign up for GitHub

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

Jump to bottom

Overall clean up #98

Merged
merged 15 commits into from
May 23, 2024
Merged

Overall clean up #98

merged 15 commits into from
May 23, 2024

Conversation

muffato
Copy link
Member

@muffato muffato commented May 18, 2024

This is a bit of cleanup of the code and parameters that I want to get in before #97 and finally reaching v1.0.0.
The Changelog mentions a tentative intermediate v0.5.0 release, though I'm not sure it will happen at all. If I can get all the work done to support draft assemblies soon enough, I may ship both as v1.0.0.

  • I've removed all the --blast* parameters because end users could not change them without breaking the pipeline.

  • I've renamed --busco to prepare the introduction of the --busco_lineages parameter in Support draft assemblies #97 .

  • I've removed --taxa_file because it didn't add anything more than --taxon.

  • I've fixed the channel factories so that they're correctly queue/value channels, and paths are properly constructed as paths (files).

  • Whitespace cleanup

And two pipeline changes:

  • More resources for blastn
  • Skip the blastn sub-workflow if there is nothing to blast

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!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • 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).

@muffato muffato self-assigned this May 18, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented May 18, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 9b1ecc3

+| ✅ 134 tests passed       |+
#| ❔  23 tests were ignored |#
!| ❗   1 tests had warnings |!

❗ Test warnings:

❔ Tests ignored:

  • files_exist - File is ignored: CODE_OF_CONDUCT.md
  • files_exist - File is ignored: assets/nf-core-blobtoolkit_logo_light.png
  • files_exist - File is ignored: docs/images/nf-core-blobtoolkit_logo_light.png
  • files_exist - File is ignored: docs/images/nf-core-blobtoolkit_logo_dark.png
  • files_exist - File is ignored: .github/ISSUE_TEMPLATE/config.yml
  • files_exist - File is ignored: .github/workflows/awstest.yml
  • files_exist - File is ignored: .github/workflows/awsfulltest.yml
  • files_exist - File is ignored: conf/igenomes.config
  • nextflow_config - Config variable ignored: manifest.name
  • nextflow_config - Config variable ignored: manifest.homePage
  • files_unchanged - File ignored due to lint config: CODE_OF_CONDUCT.md
  • files_unchanged - File ignored due to lint config: LICENSE or LICENSE.md or LICENCE or LICENCE.md
  • files_unchanged - File ignored due to lint config: .github/ISSUE_TEMPLATE/bug_report.yml
  • files_unchanged - File does not exist: .github/ISSUE_TEMPLATE/config.yml
  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
  • files_unchanged - File ignored due to lint config: .github/workflows/linting.yml
  • files_unchanged - File ignored due to lint config: assets/nf-core-blobtoolkit_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-blobtoolkit_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-blobtoolkit_logo_dark.png
  • files_unchanged - File ignored due to lint config: lib/NfcoreTemplate.groovy
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/blobtoolkit/blobtoolkit/.github/workflows/awstest.yml
  • template_strings - template_strings
  • merge_markers - merge_markers

✅ Tests passed:

Run details

  • nf-core/tools version 2.11
  • Run at 2024-05-23 14:19:44

@muffato muffato requested a review from gq1 May 20, 2024 08:26
@muffato muffato marked this pull request as ready for review May 20, 2024 08:27
@gq1
Copy link
Member

gq1 commented May 21, 2024

Looks good for me.

How you picked up all the white spaces now? nf-core linting didn't complain it before?

I think it is ok to keep --busco for busco_db and add busco_lineages. It will be easier if we have lots of users. It is fine you have made all the changes.

@muffato
Copy link
Member Author

muffato commented May 22, 2024

I think it is ok to keep --busco for busco_db and add busco_lineages. It will be easier if we have lots of users. It is fine you have made all the changes.

Thanks for raising that. The introduction of busco_lineages is indeed the only reason why I wanted to rename --busco. But here are some arguments for keeping --busco:

  • backwards compatibility
  • It would be the only database argument to be called *_db: --blastp, --blastn, --blastx and taxdump would remain without the _db suffix.
  • probably only people who would set --busco_lineages would like the change. --busco_lineages is optional and the default behaviour (use all lineages that match the species) should work for many users already. These users would just use --busco and be ok with that

@muffato
Copy link
Member Author

muffato commented May 23, 2024

I decided to remove the commit that renamed the parameter

@muffato muffato merged commit 6581abf into dev May 23, 2024
6 checks passed
@muffato muffato deleted the clean_params branch May 23, 2024 15:02
@muffato muffato mentioned this pull request Jul 18, 2024
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gq1 gq1 gq1 approved these changes

Assignees

@muffato muffato

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@muffato @gq1