fix: ignore-from-file relative to config file #701
+110
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hello! Depending on the outcome of #673 and what @ndrwnaguib thinks, this PR could be welcome and merged 👍 I haven't reviewed yet, but I can already ask:
|
jellehelsen
force-pushed
the
fix/relative-ignore-from-file
branch
4 times, most recently
from
257daf9 to
bb68cd0
Compare
jellehelsen
force-pushed
the
fix/relative-ignore-from-file
branch
from
bb68cd0 to
174a64d
Compare
|
Hi, I think I've addressed all your remarks.
|
adrienverge
reviewed
Mar 6, 2025
There was a problem hiding this comment.
Hello Jelle,
Thanks for the update!
I read how Git handles .gitignore files (yamllint takes inspiration from it) and the documentation is clear: "all paths are relative from the .gitignore file". This means:
- the purpose of this PR is right, changing current behavior makes sense ✅
- the paths of files to ignore should be interpreted relatively to the ignore file (e.g. .gitignore),
- the path of the ignore file (e.g. .gitignore) path should be interpreted *relatively to the config file (.yamllint).
In most cases, the config file (.yamllint) and the ignore file (.gitignore) are in the same directory. But in theory they can be in different places, so the pull request should be updated to reflect that.
What do you think?
Your 2 first test functions make sense and can be left where they are. But I adapted the 2 others and wrote new ones to achieve the described behavior ↑ (these ones should be appended at the end of the test file, it's more logical).
Can you check these and tell if you agree?
def test_run_with_ignore_file_in_subdir(self):
# . ← yamllint is run from here
# ├── .yamllint
# └── bin
# ├── .gitignore ← correctly references other files
# └── .ignore-key-duplicates
with open(os.path.join(self.wd, '.yamllint'), 'w') as f:
f.write('extends: default\n'
'ignore-from-file: bin/.gitignore\n'
'rules:\n'
' key-duplicates:\n'
' ignore-from-file: bin/.ignore-key-duplicates\n')
with open(os.path.join(self.wd, 'bin', '.gitignore'), 'w') as f:
f.write('*.dont-lint-me.yaml\n'
'/\n'
'!/*.lint-me-anyway.yaml\n')
with open(os.path.join(self.wd, 'bin',
'.ignore-key-duplicates'), 'w') as f:
f.write('/../ign-dup\n')
sys.stdout = StringIO()
with self.assertRaises(SystemExit):
cli.run(('-f', 'parsable', '.'))
os.remove(os.path.join(self.wd, 'bin', '.gitignore'))
os.remove(os.path.join(self.wd, 'bin', '.ignore-key-duplicates'))
out = sys.stdout.getvalue()
out = '\n'.join(sorted(out.splitlines()))
docstart = '[warning] missing document start "---" (document-start)'
keydup = '[error] duplication of key "key" in mapping (key-duplicates)'
trailing = '[error] trailing spaces (trailing-spaces)'
hyphen = '[error] too many spaces after hyphen (hyphens)'
self.assertEqual(out, '\n'.join((
'./.yamllint:1:1: ' + docstart,
'./bin/file.lint-me-anyway.yaml:3:3: ' + keydup,
'./bin/file.lint-me-anyway.yaml:4:17: ' + trailing,
'./bin/file.lint-me-anyway.yaml:5:5: ' + hyphen,
'./file-at-root.yaml:3:3: ' + keydup,
'./file-at-root.yaml:4:17: ' + trailing,
'./file-at-root.yaml:5:5: ' + hyphen,
'./ign-dup/file.yaml:4:17: ' + trailing,
'./ign-dup/file.yaml:5:5: ' + hyphen,
'./ign-dup/sub/dir/file.yaml:4:17: ' + trailing,
'./ign-dup/sub/dir/file.yaml:5:5: ' + hyphen,
'./ign-trail/file.yaml:3:3: ' + keydup,
'./ign-trail/file.yaml:4:17: ' + trailing,
'./ign-trail/file.yaml:5:5: ' + hyphen,
'./include/ign-dup/sub/dir/file.yaml:3:3: ' + keydup,
'./include/ign-dup/sub/dir/file.yaml:4:17: ' + trailing,
'./include/ign-dup/sub/dir/file.yaml:5:5: ' + hyphen,
'./s/s/ign-trail/file.yaml:3:3: ' + keydup,
'./s/s/ign-trail/file.yaml:4:17: ' + trailing,
'./s/s/ign-trail/file.yaml:5:5: ' + hyphen,
'./s/s/ign-trail/s/s/file.yaml:3:3: ' + keydup,
'./s/s/ign-trail/s/s/file.yaml:4:17: ' + trailing,
'./s/s/ign-trail/s/s/file.yaml:5:5: ' + hyphen,
'./s/s/ign-trail/s/s/file2.lint-me-anyway.yaml:3:3: ' + keydup,
'./s/s/ign-trail/s/s/file2.lint-me-anyway.yaml:4:17: ' + trailing,
'./s/s/ign-trail/s/s/file2.lint-me-anyway.yaml:5:5: ' + hyphen,
)))
def test_run_with_broken_ignore_file_in_subdir(self):
# . ← yamllint is run from here
# ├── .yamllint
# └── bin
# ├── .gitignore ← incorrectly references other files
# └── .ignore-key-duplicates
with open(os.path.join(self.wd, '.yamllint'), 'w') as f:
f.write('extends: default\n'
'ignore-from-file: bin/.gitignore\n'
'rules:\n'
' key-duplicates:\n'
' ignore-from-file: bin/.ignore-key-duplicates\n')
with open(os.path.join(self.wd, 'bin', '.gitignore'), 'w') as f:
f.write('*.dont-lint-me.yaml\n'
'/bin/\n'
'!/bin/*.lint-me-anyway.yaml\n')
with open(os.path.join(self.wd, 'bin',
'.ignore-key-duplicates'), 'w') as f:
f.write('/ign-dup\n')
sys.stdout = StringIO()
with self.assertRaises(SystemExit):
cli.run(('-f', 'parsable', '.'))
os.remove(os.path.join(self.wd, 'bin', '.gitignore'))
os.remove(os.path.join(self.wd, 'bin', '.ignore-key-duplicates'))
out = sys.stdout.getvalue()
out = '\n'.join(sorted(out.splitlines()))
docstart = '[warning] missing document start "---" (document-start)'
keydup = '[error] duplication of key "key" in mapping (key-duplicates)'
trailing = '[error] trailing spaces (trailing-spaces)'
hyphen = '[error] too many spaces after hyphen (hyphens)'
self.assertEqual(out, '\n'.join((
'./.yamllint:1:1: ' + docstart,
'./bin/file.lint-me-anyway.yaml:3:3: ' + keydup,
'./bin/file.lint-me-anyway.yaml:4:17: ' + trailing,
'./bin/file.lint-me-anyway.yaml:5:5: ' + hyphen,
'./bin/file.yaml:3:3: ' + keydup,
'./bin/file.yaml:4:17: ' + trailing,
'./bin/file.yaml:5:5: ' + hyphen,
'./file-at-root.yaml:3:3: ' + keydup,
'./file-at-root.yaml:4:17: ' + trailing,
'./file-at-root.yaml:5:5: ' + hyphen,
'./ign-dup/file.yaml:3:3: ' + keydup,
'./ign-dup/file.yaml:4:17: ' + trailing,
'./ign-dup/file.yaml:5:5: ' + hyphen,
'./ign-dup/sub/dir/file.yaml:3:3: ' + keydup,
'./ign-dup/sub/dir/file.yaml:4:17: ' + trailing,
'./ign-dup/sub/dir/file.yaml:5:5: ' + hyphen,
'./ign-trail/file.yaml:3:3: ' + keydup,
'./ign-trail/file.yaml:4:17: ' + trailing,
'./ign-trail/file.yaml:5:5: ' + hyphen,
'./include/ign-dup/sub/dir/file.yaml:3:3: ' + keydup,
'./include/ign-dup/sub/dir/file.yaml:4:17: ' + trailing,
'./include/ign-dup/sub/dir/file.yaml:5:5: ' + hyphen,
'./s/s/ign-trail/file.yaml:3:3: ' + keydup,
'./s/s/ign-trail/file.yaml:4:17: ' + trailing,
'./s/s/ign-trail/file.yaml:5:5: ' + hyphen,
'./s/s/ign-trail/s/s/file.yaml:3:3: ' + keydup,
'./s/s/ign-trail/s/s/file.yaml:4:17: ' + trailing,
'./s/s/ign-trail/s/s/file.yaml:5:5: ' + hyphen,
'./s/s/ign-trail/s/s/file2.lint-me-anyway.yaml:3:3: ' + keydup,
'./s/s/ign-trail/s/s/file2.lint-me-anyway.yaml:4:17: ' + trailing,
'./s/s/ign-trail/s/s/file2.lint-me-anyway.yaml:5:5: ' + hyphen,
)))
def test_run_with_ignore_from_file_from_subdir(self):
# .
# ├── .yamllint
# ├── .gitignore
# ├── .ignore-key-duplicates
# └── bin ← yamllint is run from here
with open(os.path.join(self.wd, '.yamllint'), 'w') as f:
f.write('extends: default\n'
'ignore-from-file: .gitignore\n'
'rules:\n'
' key-duplicates:\n'
' ignore-from-file: .ignore-key-duplicates\n')
with open(os.path.join(self.wd, '.gitignore'), 'w') as f:
f.write('*.dont-lint-me.yaml\n'
'/bin/\n'
'!/bin/*.lint-me-anyway.yaml\n')
with open(os.path.join(self.wd, '.ignore-key-duplicates'), 'w') as f:
f.write('/ign-dup\n')
sys.stdout = StringIO()
try:
os.chdir(os.path.join(self.wd, 'bin'))
with self.assertRaises(SystemExit):
cli.run(('-f', 'parsable', '.'))
finally:
os.chdir(self.wd)
os.remove(os.path.join(self.wd, '.gitignore'))
os.remove(os.path.join(self.wd, '.ignore-key-duplicates'))
out = sys.stdout.getvalue()
out = '\n'.join(sorted(out.splitlines()))
keydup = '[error] duplication of key "key" in mapping (key-duplicates)'
trailing = '[error] trailing spaces (trailing-spaces)'
hyphen = '[error] too many spaces after hyphen (hyphens)'
self.assertEqual(out, '\n'.join((
'./file.lint-me-anyway.yaml:3:3: ' + keydup,
'./file.lint-me-anyway.yaml:4:17: ' + trailing,
'./file.lint-me-anyway.yaml:5:5: ' + hyphen,
)))
def test_run_with_ignore_from_file_from_subdir_on_root_dir(self):
# .
# ├── .yamllint
# ├── .gitignore
# ├── .ignore-key-duplicates
# └── bin ← yamllint is run from here
with open(os.path.join(self.wd, '.yamllint'), 'w') as f:
f.write('extends: default\n'
'ignore-from-file: .gitignore\n'
'rules:\n'
' key-duplicates:\n'
' ignore-from-file: .ignore-key-duplicates\n')
with open(os.path.join(self.wd, '.gitignore'), 'w') as f:
f.write('*.dont-lint-me.yaml\n'
'/bin/\n'
'!/bin/*.lint-me-anyway.yaml\n')
with open(os.path.join(self.wd, '.ignore-key-duplicates'), 'w') as f:
f.write('/ign-dup\n')
sys.stdout = StringIO()
try:
os.chdir(os.path.join(self.wd, 'bin'))
with self.assertRaises(SystemExit):
cli.run(('-f', 'parsable', '..'))
finally:
os.chdir(self.wd)
os.remove(os.path.join(self.wd, '.gitignore'))
os.remove(os.path.join(self.wd, '.ignore-key-duplicates'))
out = sys.stdout.getvalue()
out = '\n'.join(sorted(out.splitlines()))
docstart = '[warning] missing document start "---" (document-start)'
keydup = '[error] duplication of key "key" in mapping (key-duplicates)'
trailing = '[error] trailing spaces (trailing-spaces)'
hyphen = '[error] too many spaces after hyphen (hyphens)'
self.assertEqual(out, '\n'.join((
'../.yamllint:1:1: ' + docstart,
'../bin/file.lint-me-anyway.yaml:3:3: ' + keydup,
'../bin/file.lint-me-anyway.yaml:4:17: ' + trailing,
'../bin/file.lint-me-anyway.yaml:5:5: ' + hyphen,
'../file-at-root.yaml:3:3: ' + keydup,
'../file-at-root.yaml:4:17: ' + trailing,
'../file-at-root.yaml:5:5: ' + hyphen,
'../ign-dup/file.yaml:4:17: ' + trailing,
'../ign-dup/file.yaml:5:5: ' + hyphen,
'../ign-dup/sub/dir/file.yaml:4:17: ' + trailing,
'../ign-dup/sub/dir/file.yaml:5:5: ' + hyphen,
'../ign-trail/file.yaml:3:3: ' + keydup,
'../ign-trail/file.yaml:4:17: ' + trailing,
'../ign-trail/file.yaml:5:5: ' + hyphen,
'../include/ign-dup/sub/dir/file.yaml:3:3: ' + keydup,
'../include/ign-dup/sub/dir/file.yaml:4:17: ' + trailing,
'../include/ign-dup/sub/dir/file.yaml:5:5: ' + hyphen,
'../s/s/ign-trail/file.yaml:3:3: ' + keydup,
'../s/s/ign-trail/file.yaml:4:17: ' + trailing,
'../s/s/ign-trail/file.yaml:5:5: ' + hyphen,
'../s/s/ign-trail/s/s/file.yaml:3:3: ' + keydup,
'../s/s/ign-trail/s/s/file.yaml:4:17: ' + trailing,
'../s/s/ign-trail/s/s/file.yaml:5:5: ' + hyphen,
'../s/s/ign-trail/s/s/file2.lint-me-anyway.yaml:3:3: ' + keydup,
'../s/s/ign-trail/s/s/file2.lint-me-anyway.yaml:4:17: ' + trailing,
'../s/s/ign-trail/s/s/file2.lint-me-anyway.yaml:5:5: ' + hyphen,
)))
def test_run_with_inaccessible_ignore_file_from_subdir(self):
# .
# ├── .yamllint
# └── bin ← yamllint is run from here
# └── .gitignore
with open(os.path.join(self.wd, '.yamllint'), 'w') as f:
f.write('extends: default\n'
'ignore-from-file: .gitignore\n')
with open(os.path.join(self.wd, 'bin', '.gitignore'), 'w') as f:
f.write('*.dont-lint-me.yaml\n'
'/bin/\n'
'!/bin/*.lint-me-anyway.yaml\n')
sys.stdout = StringIO()
sys.stderr = StringIO()
try:
os.chdir(os.path.join(self.wd, 'bin'))
with self.assertRaises(FileNotFoundError):
cli.run(('-f', 'parsable', '.'))
finally:
os.chdir(self.wd)
os.remove(os.path.join(self.wd, 'bin', '.gitignore'))| @@ -109,6 +113,11 @@ def parse(self, raw_content): | |||
| raise YamlLintConfigError( | |||
| 'invalid config: ignore-from-file should contain ' | |||
| 'filename(s), either as a list or string') | |||
| if self.config_dir is not None: | |||
| conf['ignore-from-file'] = [ | |||
| f if os.path.basename(f) != f | |||
There was a problem hiding this comment.
-
I don't understand this line: why should
fbe treated differently if it contains slashes? -
To avoid bugs is it possible not to modify the
confobject (norconf['ignore-from-file']) if it's not strictly needed? E.g. create a local variablefilepaths? -
Maybe it's worth using
os.path.relpath(filepath, start=self.config_dir)here? I'm not sure, I haven't tested. The advantage is that we wouldn't need theif self.config_dir is not None:above, becauseos.path.relpath()can be called withstart=Noneorstart=''.
| @@ -227,6 +227,7 @@ or: | |||
| ignore-from-file: [.gitignore, .yamlignore] | |||
|
|
|||
| .. note:: However, this is mutually exclusive with the ``ignore`` key. | |||
| Also, this filepath is considered relative to the config file. | |||
There was a problem hiding this comment.
I would find it clearer in a new note below. Also, we could avoid any misunderstandings like this:
Suggested change
| Also, this filepath is considered relative to the config file. | |
| .. note:: | |
| This file (e.g. ``.gitignore``) is searched relatively to the config file | |
| (``.yamllint``). The paths of files to ignore are interpreted relatively to | |
| the ignore file (e.g. ``.gitignore``). |
| '!/bin/*.lint-me-anyway.yaml\n') | ||
| parsed = config.YamlLintConfig('extends: default\n' | ||
| 'ignore-from-file: .gitignore\n') | ||
| assert parsed.is_file_ignored("/bin/ignored") is True |
Comment on lines
+785
to
+786
| if os.path.exists(os.path.join(self.wd, '.gitignore')): | ||
| os.remove(os.path.join(self.wd, '.gitignore')) |
There was a problem hiding this comment.
We can avoid this by deterministically cleaning .gitignore and .ignore-key-duplicates at the end of each test.
(see the global code snippet for tests in my main comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the problem (#673) that files mentioned in ignore-from-file can not be found when running in a subdirectory. Values in ignore-from-file are now treated as being relative to the config file.