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

Don't require an argument for nf-core schema lint #2225

Merged
merged 3 commits into from Mar 29, 2023
Merged

Conversation

sersorrel
Copy link
Member

nf-core lint and nf-core modules lint can both be called with no arguments, but nf-core schema lint requires that you pass the path to the schema. Since this will often just be at ./nextflow_schema.json, defaulting to that reduces the length of the invocation in the common case and makes the usage of all three lint commands more consistent.

I'm not 100% sure of how click.Path works – is just providing a string as the default here correct? should I be using some other Path type?

Also I don't know how the fancy images in the readme get generated, I'm hoping based on looking at the commit history for them that there's some magic github action that will kick in once I open the PR...

Fixes #1838

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated

@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Merging #2225 (8a9569b) into dev (8a9569b) will not change coverage.
The diff coverage is n/a.

❗ Current head 8a9569b differs from pull request most recent head b4e22d3. Consider uploading reports for the commit b4e22d3 to get more accurate results

@@           Coverage Diff           @@
##              dev    #2225   +/-   ##
=======================================
  Coverage   72.88%   72.88%           
=======================================
  Files          78       78           
  Lines        8372     8372           
=======================================
  Hits         6102     6102           
  Misses       2270     2270           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@awgymer
Copy link
Contributor

awgymer commented Mar 29, 2023

This all looks good to me. The default works fine as a string in my testing. Indeed I believe click.Path actually returns as string to the command and is just a class for the sake of performing some checks.

Do we need to add a test for this command that uses the default?

@sersorrel
Copy link
Member Author

I can't see any existing tests for nf-core schema lint, so I added a couple of them alongside the existing CLI tests – let me know if there are already tests and I just missed them

@Aratz Aratz self-requested a review March 29, 2023 13:24
Copy link
Contributor

@Aratz Aratz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Aratz
Copy link
Contributor

Aratz commented Mar 29, 2023

Remember to update the changelog though ;)

@sersorrel sersorrel linked an issue Mar 29, 2023 that may be closed by this pull request
@sersorrel sersorrel merged commit 14951aa into dev Mar 29, 2023
13 of 14 checks passed
@sersorrel sersorrel deleted the schema-lint-argument branch March 29, 2023 14:09
@github-actions
Copy link
Contributor

Python linting (black) is failing

To keep the code consistent with lots of contributors, we run automated code consistency checks.
To fix this CI test, please run:

  • Install black: pip install black
  • Fix formatting errors in your pipeline: black .

Once you push these changes the test should pass, and you can hide this comment 👍

We highly recommend setting up Black in your code editor so that this formatting is done automatically on save. Ask about it on Slack for help!

Thanks again for your contribution!

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.

Make nf-core lint and nf-core schema lint usage consistent
3 participants