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

skip ignored files at discovery #646

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

james-powis
Copy link

@james-powis james-powis commented Jan 14, 2024

Fixes: #399

Not sure how coverage decreased, I did not touch that file ;)

ignored files should be skipped early on to prevent other issues such as tripping on intentionally ignored broken symlinks.

There are many reasons that a file is ignored, one of those reasons is that it is a symlink which for valid reasons is broken. Without this change, this results in an [Errno 2] No such file or directory: error which can cause CI/CD to fail.

There likely is efficiencies by skipping to read in the skipped files at startup, but that is not the point of this PR.

ignored files should be skipped early on to prevent other issues such as
tripping on intentionally ignored broken symlinks.
@coveralls
Copy link

Coverage Status

coverage: 99.797% (-0.03%) from 99.822%
when pulling 5417f96 on james-powis:skip-ignored-files-on-load
into 0d7ae7b on adrienverge:master.

@adrienverge
Copy link
Owner

Thanks for contributing!

Not sure how coverage decreased, I did not touch that file ;)

Coverage decreased indeed, because changes to yamllint source code should come with tests, to make sure there won't be regression. It was requested in the original issue #399 ;)
A new test simulating the scenario of the initial message of #399 should be fine. See tests/test_config.py for how existing tests look like.

If you add a check not conf.is_file_ignored(filepath) early, should this check be removed later in yamllint execution? (I'm not 100% sure, but I remember that touching find_files_recursively() + conf.is_file_ignored() is tricky and could create problems, it needs special attention)

@james-powis
Copy link
Author

@adrienverge I did not touch linter.py so its coverage decreasing is not related to my change to cli.py and likely a related to something else....

Furthermore there is a pretty significant gordian knot related to these checks inside and out within the code. Refactoring your code is not my interest. I was just trying to contribute a fix, which this does, and your users have been complaining about for over 2 years, which I also was impacted by. I have a monkey patch in place and do not need you to accept this. I just figured I would try and help out. This is an exceptionally trivial change, and one which is exceptionally difficult to write a test for. Its just not that valuable for me to deal with that silly difficulty. However I am sure you could piece together the test modifications in my other PR if you need inspiration.

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.

Ignoring file that is a broken symbolic link produces "[Errno 2] No such file or directory"
3 participants