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

Allow existing GenomicsDb as input to incrementally update #1116

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from
Draft

Allow existing GenomicsDb as input to incrementally update #1116

wants to merge 2 commits into from

Conversation

FriederikeHanssen
Copy link
Contributor

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
  • If necessary, also make a PR on the nf-core/sarek 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 --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).

@FriederikeHanssen FriederikeHanssen changed the base branch from master to dev June 18, 2023 07:48
@nf-core nf-core deleted a comment from github-actions bot Jun 18, 2023
@github-actions
Copy link

github-actions bot commented Jun 18, 2023 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 8ab3d9c

+| ✅ 154 tests passed       |+
#| ❔   9 tests were ignored |#
!| ❗   1 tests had warnings |!

❗ Test warnings:

  • schema_description - No description provided in schema for parameter: genomicsdb_workspace

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.8
  • Run at 2023-06-18 07:49:37

maxulysse
maxulysse reviewed Jun 19, 2023
View reviewed changes
nextflow_schema.json
@@ -345,7 +351,7 @@
},
"cnvkit_reference": {
"type": "string",
"default": null,
"default": "None",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"default": "None",
"default": null,

Copy link
Member

Choose a reason for hiding this comment

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

which tools version were you using for that to happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2.8. I think, used gitpod, so should be latest release I think

Copy link
Member

Choose a reason for hiding this comment

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

@mirpedrol was this issue fixed or not with 2.8: nf-core/tools#1841

Copy link
Member

Choose a reason for hiding this comment

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

It isn't fixed yet

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why you need to define it as null in the schema, the best would be to not set any default value, but still initialize it to null in the nextflow.config

Copy link
Member

Choose a reason for hiding this comment

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

Let me check that

Copy link
Member

Choose a reason for hiding this comment

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

#1119

Copy link
Member

Choose a reason for hiding this comment

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

Should that be part of schema linting?
Just removing default None/null,"" values.
That way, no need to deal with null-> None issues at all

Copy link
Member

Choose a reason for hiding this comment

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

Adding it to linting sounds good :)

maxulysse
maxulysse reviewed Jun 19, 2023
View reviewed changes
nextflow_schema.json
@@ -528,7 +534,7 @@
"ascat_genome": {
"type": "string",
"fa_icon": "fa-solid fa-text",
"default": null,
"default": "None",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"default": "None",
"default": null,

@mirpedrol mirpedrol mentioned this pull request Jun 20, 2023
Merged
4 tasks
@maxulysse maxulysse changed the title Issue 755: Allow existing GenomicsDb as input to incrementally update Allow existing GenomicsDb as input to incrementally update Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mirpedrol mirpedrol mirpedrol left review comments

@maxulysse maxulysse maxulysse left review comments

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.
3 participants
@FriederikeHanssen @maxulysse @mirpedrol