-
Notifications
You must be signed in to change notification settings - Fork 192
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
Remove old references to CUSTOMDUMPSOFTWAREVERSIONS #2897
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add a lint test for this please?
@mirpedrol @mashehu help! |
haha, you ran into the same nf-core tools trap as @Joon-Klaps did 🙂 |
but I can have a look after I found a fix for #2890 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files☔ View full report in Codecov by Sentry. |
nf_core/lint/configs.py
Outdated
|
||
# Return a failed status if we can't find the file | ||
if not fn.is_file(): | ||
return {"failed": [f"`${file_path}` not found"]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to make this a warning? Some pipelines (like sarek) don't use these config files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to have them ignore it, imo
nf_core/lint/configs.py
Outdated
f"`{file_path}` contains `withName:{section}`, but the corresponding process is not present in any of the following workflow files: `{nf_files}`." | ||
) | ||
else: | ||
passed.append(f"both `{file_path}` and `{[str(f) for f in nf_files]} contain `{section}`.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if the list of .nf
files will be too long. Maybe a general message is enough
passed.append(f"both `{file_path}` and `{[str(f) for f in nf_files]} contain `{section}`.") | |
passed.append(f"`{section}` found in `{file_path}` and Nextflow scripts.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, but I'll add a debuggin log, so it is a bit easier to debug
…-core/tools into remove-old-customdumpsoftwareversions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Removes left over CUSTOMDUMPSOFTWAREVERSIONS references from template that has been since replaced with a dedicated special function
PR checklist
CHANGELOG.md
is updateddocs
is updated