Skip to content

Conversation

@glichtenstein
Copy link
Contributor

@glichtenstein glichtenstein commented Mar 26, 2025

PR checklist

  • This comment contains a description of changes (with reason).
  • Make sure your code lints (nf-core pipelines 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>).
  • CHANGELOG.md is updated.

Description

This PR improves the nextflow_schema.json and the schema_input.json files for the nf-core/rnaseq pipeline as part of the March 2025 JSON Schema Improvements Hackathon.

The main goals of these updates are to enhance schema consistency, improve validation error messages, and clarify the help_text and description fields. These changes aim to make the pipeline easier to configure and provide more informative feedback to users.

We followed the hackathon checklist throughout the process.

All changes were tested using:
nextflow run main.nf -profile test,docker --outdir results -stub
The schema remains valid, and no validation issues were observed.

"format": "file-path",
"exists": true,
"errorMessage": "GZIP-compressed FastQ file for reads 2 cannot contain spaces and must have extension '.fq.gz' or '.fastq.gz'",
"anyOf": [
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this anyOf part and only use the "pattern" keyword that's inside of it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on it, thanks!

"pipelines_testdata_base_path": {
"type": "string",
"minLength": 1,
"pattern": "^(https?://\\S+|/[^\\s]+|\\.{0,2}/[^\\s]+)$",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"pattern": "^(https?://\\S+|/[^\\s]+|\\.{0,2}/[^\\s]+)$",

This does not need to be a HTTPS URL and can also be a local or cloud file-path. Adding this pattern would restrict this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I wanted to make a pattern with an optional https, but it may fail and be too restrictive. I will roll it back.

Co-authored-by: Nicolas Vannieuwkerke <[email protected]>
Copy link
Contributor

@nvnieuwk nvnieuwk left a comment

Choose a reason for hiding this comment

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

Some comments, but wow this is really impressive! Well done!

@glichtenstein glichtenstein requested a review from nvnieuwk March 26, 2025 15:01
Copy link
Contributor

@nvnieuwk nvnieuwk left a comment

Choose a reason for hiding this comment

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

Good job!

@jen-reeve jen-reeve linked an issue Mar 26, 2025 that may be closed by this pull request
15 tasks
@glichtenstein glichtenstein requested a review from nvnieuwk March 27, 2025 12:52
Copy link
Contributor

@nvnieuwk nvnieuwk left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@maxulysse maxulysse left a comment

Choose a reason for hiding this comment

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

<3

@pinin4fjords
Copy link
Member

Could you please add a motivating description to the PR describing what you've done and why?

Copy link
Member

@pinin4fjords pinin4fjords 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 though!

@pinin4fjords pinin4fjords enabled auto-merge April 22, 2025 11:38
@pinin4fjords pinin4fjords merged commit a007d56 into nf-core:dev Apr 22, 2025
29 checks passed
@pinin4fjords pinin4fjords mentioned this pull request Apr 22, 2025
15 tasks
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.

JSON schema: Rnaseq

4 participants