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

Merging template updates #103

Merged
merged 29 commits into from
Mar 31, 2021
Merged

Merging template updates #103

merged 29 commits into from
Mar 31, 2021

Conversation

Leon-Bichmann
Copy link
Collaborator

@Leon-Bichmann Leon-Bichmann commented Mar 22, 2021

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 - add to the software_versions process and a regex to scrape_software_versions.py
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/diaproteomics branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint .).
  • Ensure the test suite passes (nextflow run . -profile test,docker).
  • 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).

@Leon-Bichmann Leon-Bichmann changed the base branch from master to dev March 22, 2021 14:10
@nf-core nf-core deleted a comment from github-actions bot Mar 23, 2021
@Leon-Bichmann
Copy link
Collaborator Author

Ok, this is good to be merged once the nfcore schema changes are included into the new template.

@Leon-Bichmann
Copy link
Collaborator Author

@maxulysse I am having problems with the logo - apparently it clashes with the template. Do you see whats the problem?

files_unchanged: docs/images/nf-core-diaproteomics_logo.png does not match …

@ewels
Copy link
Member

ewels commented Mar 24, 2021

@maxulysse I am having problems with the logo - apparently it clashes with the template. Do you see whats the problem?

files_unchanged: docs/images/nf-core-diaproteomics_logo.png does not match …

We changed the logo a little to have a white background so that it shows up on GitHub with dark mode.

Just run nf-core lint . --fix files_unchanged and the tool will replace your logo with the file it expects 👍🏻

@github-actions
Copy link

github-actions bot commented Mar 24, 2021

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit a16dc74

+| ✅ 180 tests passed       |+
!| ❗  23 tests had warnings |!

❗ Test warnings:

  • conda_env_yaml - Conda dep outdated: conda-forge::python=3.6.10, 3.9.2 available
  • conda_env_yaml - Conda dep outdated: conda-forge::markdown=3.1.1, 3.3.4 available
  • conda_env_yaml - Conda dep outdated: conda-forge::pymdown-extensions=6.0, 8.1.1 available
  • conda_env_yaml - Conda dep outdated: conda-forge::pygments=2.5.2, 2.8.1 available
  • conda_env_yaml - Conda dep outdated: anaconda::openjdk=8.0.152, 11.0.6 available
  • conda_env_yaml - Conda dep outdated: conda-forge::r-base=4.0.2, 4.0.3 available
  • conda_env_yaml - Conda dep outdated: bioconda::openms-thirdparty=2.5.0, 2.6.0 available
  • conda_env_yaml - Conda dep outdated: bioconda::pyprophet=2.1.4, 2.1.10 available
  • conda_env_yaml - Conda dep outdated: conda-forge::networkx=2.4, 2.5 available
  • conda_env_yaml - Conda dep outdated: bioconda::easypqp=0.1.8, 0.1.11 available
  • conda_env_yaml - Conda dep outdated: conda-forge::r-phangorn=2.5.5, 2.6.2 available
  • conda_env_yaml - Conda dep outdated: conda-forge::r-tidyr=1.1.2, 1.1.3 available
  • conda_env_yaml - Conda dep outdated: conda-forge::r-dplyr=1.0.2, 1.0.5 available
  • conda_env_yaml - Conda dep outdated: conda-forge::r-ggplot2=3.3.2, 3.3.3 available
  • conda_env_yaml - Conda dep outdated: conda-forge::r-gplots=3.1.0, 3.1.1 available
  • conda_env_yaml - Conda dep outdated: conda-forge::r-biocmanager=1.30.10, 1.30.12 available
  • conda_env_yaml - Conda dep outdated: conda-forge::r-dbi=1.1.0, 1.1.1 available
  • conda_env_yaml - Conda dep outdated: conda-forge::r-rcpp=1.0.5, 1.0.6 available
  • conda_env_yaml - Conda dep outdated: conda-forge::r-rsqlite=2.2.1, 2.2.5 available
  • conda_env_yaml - Conda dep outdated: conda-forge::r-zoo=1.8_8, 1.8_9 available
  • conda_env_yaml - Conda dep outdated: bioconductor-MSstats=3.20.1, 3.22.1 available
  • conda_env_yaml - Conda dep outdated: bioconductor-mzr=2.24.0, 2.24.1 available
  • conda_env_yaml - Conda dep outdated: bioconductor-biocparallel=1.24.0, 1.24.1 available

✅ Tests passed:

Run details

  • nf-core/tools version 1.13.3
  • Run at 2021-03-31 17:31:10

@Leon-Bichmann
Copy link
Collaborator Author

ok great now it passed

@Leon-Bichmann
Copy link
Collaborator Author

it seems the last test in the CI doesn't complete even everything is done already:
Run workflow tests Expected — Waiting for status to be reported

@ewels
Copy link
Member

ewels commented Mar 24, 2021

Looks like you have a few lint warnings that should probably be cleared up:

  • pipeline_todos - TODO string in main.nf: Add any reference files that are needed
  • schema_params - Schema param irts not found from nextflow config
  • schema_params - Schema param input_sheet_dda not found from nextflow config
  • schema_params - Schema param config_profile_name not found from nextflow config

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.

Looks good! Couple of minor things but not much.

conf/test.config Outdated
@@ -44,4 +44,6 @@ params {
input_sheet_dda = 'https://raw.githubusercontent.com/nf-core/test-datasets/diaproteomics/dda_sheet.tsv'
input = 'https://raw.githubusercontent.com/nf-core/test-datasets/diaproteomics/sample_sheet.tsv'
unimod = 'https://raw.githubusercontent.com/nf-core/test-datasets/diaproteomics/unimod.xml'
// Ignore `--input` as otherwise the parameter validation will throw an error
schema_ignore_params = 'genomes,input_paths,input'
Copy link
Member

Choose a reason for hiding this comment

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

As with other review, I don't think that you're using input_paths (or genomes?)

@@ -41,4 +41,6 @@ params {
unimod = 'https://raw.githubusercontent.com/nf-core/test-datasets/diaproteomics/unimod.xml'
input_sheet_dda = 'https://raw.githubusercontent.com/nf-core/test-datasets/diaproteomics/dda_sheet_full.tsv'
irts = 'https://raw.githubusercontent.com/nf-core/test-datasets/diaproteomics/irt_sheet_full.tsv'
// Ignore `--input` as otherwise the parameter validation will throw an error
schema_ignore_params = 'genomes,input_paths,input'
Copy link
Member

Choose a reason for hiding this comment

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

☝🏻

nextflow.config Outdated
@@ -85,6 +84,9 @@ params {
config_profile_description = false
config_profile_contact = false
config_profile_url = false
validate_params = true
show_hidden_params = false
schema_ignore_params = 'genomes,input_paths'
Copy link
Member

Choose a reason for hiding this comment

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

Probably don't need this?

Also, input should technically be null if not set by default instead of false (though not sure that it makes any difference at this point).

Copy link
Member

Choose a reason for hiding this comment

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

As with mhcquant, I think that you can strip all mention of genomes, right?

Co-authored-by: Phil Ewels <[email protected]>
@Leon-Bichmann
Copy link
Collaborator Author

@ewels - Alright everything passed, but I guess I wait to include the 1.13.3 template updates still

@Leon-Bichmann Leon-Bichmann requested a review from ewels March 25, 2021 08:55
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.

As with mhcquant, changes all look good now I think - just a question of whether you want to strip all mention of igenomes and genomes.

nextflow.config Outdated
@@ -85,6 +84,9 @@ params {
config_profile_description = false
config_profile_contact = false
config_profile_url = false
validate_params = true
show_hidden_params = false
schema_ignore_params = 'genomes,input_paths'
Copy link
Member

Choose a reason for hiding this comment

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

As with mhcquant, I think that you can strip all mention of genomes, right?

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.

One left that I can see from a quick scan

Comment on lines 375 to 376
"igenomes_ignore": {
"type": "boolean",
Copy link
Member

Choose a reason for hiding this comment

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

Can delete

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah ok, now I got it I though it was about igenomes_base which was already deleted

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the parameter group "reference_genome_options" will be emtpy though... and if I remove the whole parameter group linting fails.

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.

Looking great! I'm happy re: the template merge. I think that the schema could do with a little love still but will pop in my own PR to address some of this stuff now instead of leaving more comments.

@ewels ewels merged commit f3af78d into nf-core:dev Mar 31, 2021
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.

3 participants