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

Fix #368 #370

Merged
merged 8 commits into from
Mar 23, 2023
Merged

Fix #368 #370

merged 8 commits into from
Mar 23, 2023

Conversation

drpatelh
Copy link
Member

Closes #368

@github-actions
Copy link

github-actions bot commented Mar 22, 2023 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit e5d5293

+| ✅ 147 tests passed       |+
#| ❔   4 tests were ignored |#
!| ❗   5 tests had warnings |!

❗ Test warnings:

  • files_exist - File not found: assets/multiqc_config.yml
  • files_exist - File not found: conf/igenomes.config
  • files_exist - File not found: lib/WorkflowViralrecon.groovy
  • nextflow_config - Config manifest.version should end in dev: '2.6.0'
  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your prefered methods description, e.g. add publication citation for this pipeline

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.7.2
  • Run at 2023-03-23 10:18:01

@drpatelh drpatelh mentioned this pull request Mar 22, 2023
Closed
ewels
ewels approved these changes Mar 22, 2023
View reviewed changes
Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Don't think you need the q=params in the URL. I guess it's left over from a website search and don't think it's doing anything.

lib/NfcoreTemplate.groovy Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
@wutron
Copy link

wutron commented Mar 22, 2023 •
edited
Loading

Unrelated to the fix, but I think there is an issue in the subworkflows as well, e.g. remove /main.
https://github.com/drpatelh/nf-core-viralrecon/blob/fixes/workflows/illumina.nf#L99

@drpatelh
Copy link
Member Author

Sorry, decided to push some other changes here @wutron because it was quicker

Unrelated to the fix, but I think there is an issue in the subworkflows as well, e.g. remove /main.
https://github.com/drpatelh/nf-core-viralrecon/blob/fixes/workflows/illumina.nf#L99

This should be fine with or without the main? The former was the previous convention.

@wutron
Copy link

wutron commented Mar 22, 2023

Sorry, decided to push some other changes here @wutron because it was quicker

Unrelated to the fix, but I think there is an issue in the subworkflows as well, e.g. remove /main.
https://github.com/drpatelh/nf-core-viralrecon/blob/fixes/workflows/illumina.nf#L99

This should be fine with or without the main? The former was the previous convention.

Should https://github.com/drpatelh/nf-core-viralrecon/blob/fixes/workflows/illumina.nf#L99 have /main then?

@wutron
Copy link

wutron commented Mar 22, 2023

Closes #368

Confirm fixed. Sorry for delay. Had to re-process several million reads.

@drpatelh
Copy link
Member Author

Should https://github.com/drpatelh/nf-core-viralrecon/blob/fixes/workflows/illumina.nf#L99 have /main then?

It doesn't matter whether it does or not. Nextflow will pick up the main,nf script by default. I guess we should be consistent with the convention we use though 👍🏽

@drpatelh
Copy link
Member Author

Should https://github.com/drpatelh/nf-core-viralrecon/blob/fixes/workflows/illumina.nf#L99 have /main then?

Fixed e765818

jfy133
jfy133 reviewed Mar 23, 2023
View reviewed changes
lib/NfcoreTemplate.groovy Outdated Show resolved Hide resolved
jfy133
jfy133 reviewed Mar 23, 2023
View reviewed changes
lib/NfcoreTemplate.groovy Outdated Show resolved Hide resolved
README.md
@@ -98,6 +98,8 @@ A number of improvements were made to the pipeline recently, mainly with regard

4. Start running your own analysis!

> - Please provide pipeline parameters via the CLI or Nextflow `-params-file` option. Custom config files including those provided by the `-c` Nextflow option can be used to provide any configuration except for parameters; see [docs](https://nf-co.re/usage/configuration#custom-configuration-files).
Copy link
Member

Choose a reason for hiding this comment

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

I would not add this here, if you would be new to the pipeline you wouldn't really know passing parameters to pipelines via -c exists. You should point instead to the website IF you want to include something: https://nf-co.re/docs/usage/configuration#custom-configuration-files

If you've used the pipeline before, you should know to look at the website therefore you wouldn't be looking at the README as a quick reference

You're risking adding more and more fluff to what is to be very basic intro docs which @grst just tried to remove ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Whatever is written in the pipeline README is rendered on the website?

I agree with trimming down the boilerplate but this is quite important. A number of users have been stung by it and complained that there is no clear documentation for this in the pipeline. Think we should have it on the website in more detail as well as the pipeline in as many prominent locations as possible. Not sure we should compromise on this if it saves a user countless hours figuring out why their parameters are not being passed to the pipeline.

Copy link
Member Author

Choose a reason for hiding this comment

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

See nf-core/rnaseq#894 for context

Another user spoke to me about this at the Festival of Genomics.

Copy link
Member

Choose a reason for hiding this comment

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

Ok fair enough. Can you please copy this over to the template though ;-*

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm scrambling to get 2 pipelines out this week and have a bunch of other stuff I need to do before the Hackathon. At the very least I will create an issue pointing to the commits so it can be easily added during the Hackathon.

Once we have revised the website docs we can always trim down the text but let's keep it as it is for now.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would also be possible to add a check for this, see
nf-core/tools#2184

@drpatelh drpatelh merged commit 5daf5a8 into nf-core:dev Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfy133 jfy133 jfy133 left review comments

@grst grst grst left review comments

@ewels ewels ewels approved these changes

@maxulysse maxulysse maxulysse approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@drpatelh @wutron @ewels @maxulysse @grst @jfy133