-
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
Add linting for ifEmpty(null) #3411
base: dev
Are you sure you want to change the base?
Conversation
@nf-core-bot fix linting |
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.
Hi @lmReef, thanks for contributing!
It is looking good, but we should also add a test for this. You can find samples for other linting tests in tests/pipelines/lint
, I think https://github.com/nf-core/tools/blob/main/tests/pipelines/lint/test_template_strings.py will be useful for this case
ignore = [".git"] | ||
if Path(root_dir, ".gitignore").is_file(): | ||
with open(Path(root_dir, ".gitignore"), encoding="latin1") as fh: | ||
for line in fh: | ||
ignore.append(Path(line.strip().rstrip("/")).name) |
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.
We are not ignoring paths on .gitignore
for other linting tests, I think it's a good idea, but I am wondering if we should add it to all tests for consistency.
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.
I see we have an issue for that #3385 let's leave it for a separate PR
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.
sounds good, I'll remove it from here and maybe have a look at a more general implementation in a different pr
files[:] = [f for f in files if not fnmatch.fnmatch(str(Path(root, f)), i)] | ||
for fname in files: | ||
try: | ||
with open(Path(root, fname), encoding="latin1") as fh: |
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.
I don't think we have latin1 enconding
with open(Path(root, fname), encoding="latin1") as fh: | |
with open(Path(root, fname)) as fh: |
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.
interestingly without either encoding="latin1"
or errors="ignore"
it fails to decode files both during the pytest and when testing the linting on a new pipeline.
I took this from the file pipelines/lint/pipeline_todos.py. Is this a me issue or should I add latin1
back in?
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.
actually I just realized template_strings.py uses encoding="latin1"
for reading files too, I'll add it back in for now
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.
my bad, let's add encoding="latin1"
back, thanks!
awesome thanks for the feedback, I'll make those changes |
Add pipeline and subworkflow linting for ifEmpty(null)
Resolves issue #2506
Description from original issue
Warning message example:
Implementation based on
pipeline_todos
as it's very similar (link to main file)Uses regex pattern
ifEmpty\(\s*null\s*\)
PR checklist
CHANGELOG.md
is updateddocs
is updated