adrienverge / yamllint

A linter for YAML files.
GNU General Public License v3.0
2.9k stars 278 forks source link

handle symlinks in a semi dry manner #647

Open james-powis opened 10 months ago

james-powis commented 10 months ago

The Goal of this PR is to incorporate a fairly clean way to handle symlinks.

Symlinks are exceptionally problematic, because:

This solution aims to:

This solution falls flat to be a good solution because:

coveralls commented 10 months ago

Coverage Status

coverage: 99.797% (-0.03%) from 99.822% when pulling ce64fc24e5c0cc68598ddb17941db9cc73b2302c on james-powis:symlink-dry into 3cb3a2038569370a2e44bcda28a378dac96c6c10 on adrienverge:master.

adrienverge commented 10 months ago

This pull request is not reviewable as is: function-local code style not respected, quotes, inclusion of yamllint/.cli.py.swp (congrats for using Vim by the way :wink:), remains of your tests like maxDiff = None, independant changes like set() that should have their own commit / review, etc.

But I understand that it's intended, and you want to start a discussion about symbolic links. To be honest I have a hard time understanding your commit message (e.g. the "strategies"), and what "this solution" consists in. You should describe it.

And my current recommendation is to skip them entirely.

I don't think all yamllint users would agree, this would be a breaking change.

james-powis commented 10 months ago

The goal of this PR was not to actually get this accepted, it was to demonstrate one of the few problems symlinks present. Especially around excluding... Do you apply exclude the symlink or the target, or both, none? How do you handle the reporting of the lint issue, do you report the link, or the target?. How do you prevent 100 million symlinks to the same file not result in the running of yamllint 100 million times.

Ultimately, if the file should be linted, it should be linted at the source not the one of potentially numerous links to it. By excluding the symlink entirely, the users of yamllint have the option to target the target via include if so desired file. But that is just my opinion.