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

Avoid runtime exception caused by newer pathspec #548

Closed
wants to merge 1 commit into from

Conversation

ssbarnea
Copy link
Sponsor Contributor

@ssbarnea ssbarnea commented Mar 16, 2023

Some changes made in the minor pathspec update recently released break yamllint. This makes the code work with newer version too.

  • pathspec==0.11.0 worked
  • pathspec==0.11.1 no longer works, fixed by this change

See: https://github.com/ansible/ansible-lint/actions/runs/4439000759/jobs/7790864220?pr=3194

@coveralls
Copy link

coveralls commented Mar 16, 2023

Coverage Status

Coverage: 99.184%. Remained the same when pulling 5f60dba on ssbarnea:fix/pathlib into 4046563 on adrienverge:master.

@adrienverge
Copy link
Owner

Hello @ssbarnea, thanks for the fix! Could you give a small example of a directory of YAML files that triggers the bug with pathspec 0.11.1?

@ssbarnea
Copy link
Sponsor Contributor Author

I will try to add a test for it. I was hoping you will forget to ask me about a test ;)

@adrienverge
Copy link
Owner

The test might not be needed? It depends on what breaks. I'm curious about a simple example here, and we'll see whether a test is needed.

@ssbarnea
Copy link
Sponsor Contributor Author

@adrienverge I added the test, it is needed, if you remove the code change the test will fail with exception. It was a good call to ask me to add a test.

adrienverge added a commit that referenced this pull request Mar 16, 2023
As reported in #548, there
might be a problem with	pathspec 0.11.1 which does't allow calling
`match_file()` with argument `None` anymore.

The `linter.run()` function shouldn't call
`YamlLintConfig.is_file_ignored(None)` anyway.
@adrienverge
Copy link
Owner

I'm sorry to ask again, but please, could you provide a real-life example of a directory structure + yamllint conf that causes a failure? The added test does't explain it, not why YamlLintConfig.is_file_ignored() is called with argument None (which could be the real problem -- maybe #549 is a better fix).

@ssbarnea
Copy link
Sponsor Contributor Author

That happens for anyone trying to call run_yamllint without passing the last optional argument, which defaults to None.

from yamllint.linter import run as run_yamllint
from yamllint.config import YamlLintConfig

run_yamllint("", YamlLintConfig("ignore: foo.txt"))

Sorry for having to mention it but, lack of typing support in yamllint combined with lack of testing for invalid arguments, can cause bugs like this.

The linter is expected to allow linting content that does not come from a specific file (stdin), but once you have a config file that has an ignore: section, it chokes when it ends having to check if the file without a name is excluded.

That is the kind of bug that can be prevented when using typing/mypy.

I hope this helps explaining it.

@adrienverge
Copy link
Owner

OK, so from my understanding this is not a bug with command-line tool yamllint. It's a bug that only happens when using the Python lib yamllint. Right?

The current version of this pull request still doesn't seem appropriate to me, checking that is_file_ignored() doesn't ignore a file None doesn't make sense. The right thing to do is make sure is_file_ignored() is only called with strings (file paths). I'll merge #549 for this purpose.

adrienverge added a commit that referenced this pull request Mar 19, 2023
As reported in #548, there
might be a problem with	pathspec 0.11.1 which does't allow calling
`match_file()` with argument `None` anymore.

The `linter.run()` function shouldn't call
`YamlLintConfig.is_file_ignored(None)` anyway.
@alanbchristie
Copy link

If yesterday's linter fix 15f8204 is a solution (it looks like it is) then can we expect a new PyPI release soon?

@adrienverge
Copy link
Owner

Hello @alanbchristie, I just released yamllint 1.30.0, which includes the fix 👍

@alanbchristie
Copy link

Thanks - that's now fixed it!

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.

None yet

4 participants