closes #360, add support for ignore-from-file key option
#363
Conversation
ndrwnaguib
force-pushed
the
master
branch
2 times, most recently
from
3231453 to
0c1762b
Compare
There was a problem hiding this comment.
Hello @andrewnaguib, thanks for contributing!
Before review starts, can you rebase on latest master, so that tests are run? We just switched today from Travis to GitHub Actions. Also you can use a conventional commit title for easier processing (e.g. git blame/log/bisect...), see examples in this repo commit history; and please add the full URL https://github.com/adrienverge/yamllint/issues/360 in the commit message.
About the feature, I don't know what to think about accepting both filenames and lists of filenames. I think it would be the first yamllint option that is ambiguous, in the sense that unprecise types are accepted. I agree it's more user-friendly. Feedback from other reviewers is welcome!
Have you tested all possible corner cases? E.g. incorrect files passed to ignore-from-file? Multiple ignore-from-file that override themselves? Etc.
yamllint/config.py
Outdated
| if ('ignore' in conf) ^ ('ignore-from-file' in conf): | ||
|
|
||
| if 'ignore-from-file' in conf: | ||
| if isinstance(conf['ignore-from-file'], str): | ||
| # if str enclose in list | ||
| conf['ignore-from-file'] = [conf['ignore-from-file']] | ||
| self.ignore = pathspec.PathSpec.from_lines( | ||
| 'gitwildmatch', | ||
| _concat_files_content( | ||
| conf['ignore-from-file'] | ||
| ).splitlines() | ||
| ) | ||
|
|
||
| elif 'ignore' in conf: | ||
| if not isinstance(conf['ignore'], str): | ||
| raise YamlLintConfigError( | ||
| 'invalid config: ignore should contain file patterns') | ||
| self.ignore = pathspec.PathSpec.from_lines( | ||
| 'gitwildmatch', conf['ignore'].splitlines()) | ||
| elif 'ignore' in conf and 'ignore-from-file' in conf: | ||
| raise YamlLintConfigError( | ||
| 'invalid config: ' | ||
| 'ignore and ignore-from-file keys cannot be used together' | ||
| ) |
There was a problem hiding this comment.
This form is shorter, maybe it's also more readable:
if 'ignore' in conf and 'ignore-from-file' in conf:
raise YamlLintConfigError(...)
elif 'ignore-from-file' in conf:
pass
elif 'ignore' in conf:
passThere was a problem hiding this comment.
Before review starts, can you rebase on latest
master, so that tests are run? We just switched today from Travis to GitHub Actions. Also you can use a conventional commit title for easier processing (e.g.git blame/log/bisect...), see examples in this repo commit history; and please add the full URLhttps://github.com/adrienverge/yamllint/issues/360in the commit message.
You're welcome!
Sure, fixed that.
I don't know what to think about accepting both filenames and lists of filenames.
I wasn't sure too, just wanted to keep balance between user-friendliness and precise types. However, I put the string into list in parsing. This will help in removing it in case the disadvantages weighs the gains.
Have you tested all possible corner cases? E.g. incorrect files passed to
ignore-from-file? Multipleignore-from-filethat override themselves? Etc.
Yes, for incorrect files passed to ignore-from-file. But, for duplicate keys I leave it on yaml since you use it, which will use only the latest key in the file. If you expect something else, please let me know so that I refactor the code to conform with it.
ndrwnaguib
force-pushed
the
master
branch
2 times, most recently
from
6d667fc to
db090c8
Compare
There was a problem hiding this comment.
Hello @andrewnaguib, sorry for the delay.
The PR still looks good but there are a few things to fix.
Have you tested all possible corner cases? E.g. incorrect files passed to
ignore-from-file? Multipleignore-from-filethat override themselves? Etc.Yes, for incorrect files passed to
ignore-from-file. But, for duplicate keys I leave it onyamlsince you use it, which will use only the latest key in the file. If you expect something else, please let me know so that I refactor the code to conform with it.
Sorry, I meant that automatic tests (inside tests/test_config.py) should test those corner cases. For instance, if .gitignore contains invalid content, or if .gitignore doesn´t exist, etc. Please think about what could go wrong, it should be tested.
For duplicate keys between two .gitignore files I agree PyYAML will keep the latest one. But it would be better if unit tests checked that, no? (in case of conflicting values, the latest one takes precedence for example)
About the commit message, really we try to follow Linux commit message guidelines, you can look at Git history to see examples of what to do (maybe you mixed commit message and commit title in my previous message).
|
Hi @adrienverge, it's been a while! Sorry about that. I've worked on your recent comments. Let me know what do you think. |
|
@adrienverge could you please review this PR from @ndrwnaguib ? Thank you both for the work! |
There was a problem hiding this comment.
Hello @ndrwnaguib and @politician, sorry for the long delay.
I've finally found some time to work on this, and have chosen to reworked the pull request myself to avoid more review ping-pong. Here are my changes:
--- a/docs/configuration.rst
+++ b/docs/configuration.rst
@@ -190,12 +190,17 @@ Here is a more complex example:
*.ignore-trailing-spaces.yaml
ascii-art/*
-You can also use the ``.gitignore`` (or any file or files) directly through:
+You can also use the ``.gitignore`` file (or any list of files) through:
.. code-block:: yaml
- # For all rules
- ignore-from-file: [.gitignore, .yamlignore] # or: ignore-from-file: .gitignore
+ ignore-from-file: .gitignore
+
+or:
+
+.. code-block:: yaml
+
+ ignore-from-file: [.gitignore, .yamlignore]
.. note:: However, this is mutually exclusive with the ``ignore`` key.
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -430,33 +430,10 @@ class ExtendedLibraryConfigTestCase(unittest.TestCase):
self.assertEqual(new.rules['empty-lines']['max-end'], 0)
-class IgnoreConfig(unittest.TestCase):
- def test_mutually_exclusive_ignore_keys(self):
- self.assertRaises(
- YamlLintConfigError,
- config.YamlLintConfig, 'extends: default\n'
- 'ignore-from-file: .gitignore\n'
- 'ignore: |\n'
- ' *.dont-lint-me.yaml\n'
- ' /bin/\n')
-
- def test_ignore_from_file_not_exist(self):
- self.assertRaises(
- FileNotFoundError,
- config.YamlLintConfig, 'extends: default\n'
- 'ignore-from-file: not_found_file\n')
-
- def test_ignore_from_file_incorrect_type(self):
- self.assertRaises(
- YamlLintConfigError,
- config.YamlLintConfig, 'extends: default\n'
- 'ignore-from-file: 0\n')
-
-
-class IgnorePathConfigTestCase(unittest.TestCase):
+class IgnoreConfigTestCase(unittest.TestCase):
@classmethod
def setUpClass(cls):
- super(IgnorePathConfigTestCase, cls).setUpClass()
+ super().setUpClass()
bad_yaml = ('---\n'
'- key: val1\n'
@@ -476,22 +453,6 @@ class IgnorePathConfigTestCase(unittest.TestCase):
's/s/ign-trail/file.yaml': bad_yaml,
's/s/ign-trail/s/s/file.yaml': bad_yaml,
's/s/ign-trail/s/s/file2.lint-me-anyway.yaml': bad_yaml,
-
- '.yamllint': 'ignore: |\n'
- ' *.dont-lint-me.yaml\n'
- ' /bin/\n'
- ' !/bin/*.lint-me-anyway.yaml\n'
- '\n'
- 'extends: default\n'
- '\n'
- 'rules:\n'
- ' key-duplicates:\n'
- ' ignore: |\n'
- ' /ign-dup\n'
- ' trailing-spaces:\n'
- ' ignore: |\n'
- ' ign-trail\n'
- ' !*.lint-me-anyway.yaml\n',
})
cls.backup_wd = os.getcwd()
@@ -499,13 +460,38 @@ class IgnorePathConfigTestCase(unittest.TestCase):
@classmethod
def tearDownClass(cls):
- super(IgnorePathConfigTestCase, cls).tearDownClass()
+ super().tearDownClass()
os.chdir(cls.backup_wd)
shutil.rmtree(cls.wd)
- def test_run_with_ignored_path(self):
+ def test_mutually_exclusive_ignore_keys(self):
+ self.assertRaises(
+ YamlLintConfigError,
+ config.YamlLintConfig, 'extends: default\n'
+ 'ignore-from-file: .gitignore\n'
+ 'ignore: |\n'
+ ' *.dont-lint-me.yaml\n'
+ ' /bin/\n')
+
+ def test_ignore_from_file_not_exist(self):
+ self.assertRaises(
+ FileNotFoundError,
+ config.YamlLintConfig, 'extends: default\n'
+ 'ignore-from-file: not_found_file\n')
+
+ def test_ignore_from_file_incorrect_type(self):
+ self.assertRaises(
+ YamlLintConfigError,
+ config.YamlLintConfig, 'extends: default\n'
+ 'ignore-from-file: 0\n')
+ self.assertRaises(
+ YamlLintConfigError,
+ config.YamlLintConfig, 'extends: default\n'
+ 'ignore-from-file: [0]\n')
+
+ def test_no_ignore(self):
sys.stdout = StringIO()
with self.assertRaises(SystemExit):
cli.run(('-f', 'parsable', '.'))
@@ -513,82 +499,109 @@ class IgnorePathConfigTestCase(unittest.TestCase):
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,
+ './file.dont-lint-me.yaml:3:3: ' + keydup,
+ './file.dont-lint-me.yaml:4:17: ' + trailing,
+ './file.dont-lint-me.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(self):
+ with open(os.path.join(self.wd, '.yamllint'), 'w') as f:
+ f.write('extends: default\n'
+ 'ignore: |\n'
+ ' *.dont-lint-me.yaml\n'
+ ' /bin/\n'
+ ' !/bin/*.lint-me-anyway.yaml\n'
+ 'rules:\n'
+ ' key-duplicates:\n'
+ ' ignore: |\n'
+ ' /ign-dup\n'
+ ' trailing-spaces:\n'
+ ' ignore: |\n'
+ ' ign-trail\n'
+ ' !*.lint-me-anyway.yaml\n')
-class IgnoreFromFileConfigTestCase(unittest.TestCase):
- @classmethod
- def setUpClass(cls):
- super(IgnoreFromFileConfigTestCase, cls).setUpClass()
-
- bad_yaml = ('---\n'
- '- key: val1\n'
- ' key: val2\n'
- '- trailing space \n'
- '- lonely hyphen\n')
-
- ignored_files = ('*.dont-lint-me.yaml\n'
- '/bin/\n'
- '!/bin/*.lint-me-anyway.yaml\n')
- cls.wd = build_temp_workspace({
- 'bin/file.lint-me-anyway.yaml': bad_yaml,
- 'bin/file.yaml': bad_yaml,
- 'file-at-root.yaml': bad_yaml,
- 'file.dont-lint-me.yaml': bad_yaml,
- 'ign-dup/file.yaml': bad_yaml,
- 'ign-dup/sub/dir/file.yaml': bad_yaml,
- 'ign-trail/file.yaml': bad_yaml,
- 'include/ign-dup/sub/dir/file.yaml': bad_yaml,
- 's/s/ign-trail/file.yaml': bad_yaml,
- 's/s/ign-trail/s/s/file.yaml': bad_yaml,
- 's/s/ign-trail/s/s/file2.lint-me-anyway.yaml': bad_yaml,
- '.gitignore': ignored_files,
-
- '.yamllint': 'ignore-from-file: .gitignore\n'
- 'extends: default\n'
- })
+ sys.stdout = StringIO()
+ with self.assertRaises(SystemExit):
+ cli.run(('-f', 'parsable', '.'))
- cls.backup_wd = os.getcwd()
- os.chdir(cls.wd)
+ out = sys.stdout.getvalue()
+ out = '\n'.join(sorted(out.splitlines()))
- @classmethod
- def tearDownClass(cls):
- super(IgnoreFromFileConfigTestCase, cls).tearDownClass()
+ 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)'
- os.chdir(cls.backup_wd)
+ 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: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:5:5: ' + hyphen,
+ './s/s/ign-trail/s/s/file.yaml:3:3: ' + keydup,
+ './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,
+ )))
- shutil.rmtree(cls.wd)
+ def test_run_with_ignore_from_file(self):
+ 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, '.gitignore'), 'w') as f:
+ f.write('*.dont-lint-me.yaml\n'
+ '/bin/\n'
+ '!/bin/*.lint-me-anyway.yaml\n')
- def test_run_with_ignored_from_file(self):
sys.stdout = StringIO()
with self.assertRaises(SystemExit):
cli.run(('-f', 'parsable', '.'))
@@ -632,52 +645,16 @@ class IgnoreFromFileConfigTestCase(unittest.TestCase):
'./s/s/ign-trail/s/s/file2.lint-me-anyway.yaml:5:5: ' + hyphen,
)))
-
-class IgnoreFromMultiFileConfigTestCase(unittest.TestCase):
- @classmethod
- def setUpClass(cls):
- super(IgnoreFromMultiFileConfigTestCase, cls).setUpClass()
-
- bad_yaml = ('---\n'
- '- key: val1\n'
- ' key: val2\n'
- '- trailing space \n'
- '- lonely hyphen\n')
-
- ignored_files_file_1 = ('*.dont-lint-me.yaml\n'
- '/bin/\n')
- ignored_files_file_2 = ('!/bin/*.lint-me-anyway.yaml\n')
- cls.wd = build_temp_workspace({
- 'bin/file.lint-me-anyway.yaml': bad_yaml,
- 'bin/file.yaml': bad_yaml,
- 'file-at-root.yaml': bad_yaml,
- 'file.dont-lint-me.yaml': bad_yaml,
- 'ign-dup/file.yaml': bad_yaml,
- 'ign-dup/sub/dir/file.yaml': bad_yaml,
- 'ign-trail/file.yaml': bad_yaml,
- 'include/ign-dup/sub/dir/file.yaml': bad_yaml,
- 's/s/ign-trail/file.yaml': bad_yaml,
- 's/s/ign-trail/s/s/file.yaml': bad_yaml,
- 's/s/ign-trail/s/s/file2.lint-me-anyway.yaml': bad_yaml,
- '.gitignore': ignored_files_file_1,
- '.yamlignore': ignored_files_file_2,
-
- '.yamllint': 'ignore-from-file: [.gitignore, .yamlignore]\n'
- 'extends: default\n'
- })
-
- cls.backup_wd = os.getcwd()
- os.chdir(cls.wd)
-
- @classmethod
- def tearDownClass(cls):
- super(IgnoreFromMultiFileConfigTestCase, cls).tearDownClass()
-
- os.chdir(cls.backup_wd)
-
- shutil.rmtree(cls.wd)
-
def test_run_with_ignored_from_file(self):
+ with open(os.path.join(self.wd, '.yamllint'), 'w') as f:
+ f.write('ignore-from-file: [.gitignore, .yamlignore]\n'
+ 'extends: default\n')
+ with open(os.path.join(self.wd, '.gitignore'), 'w') as f:
+ f.write('*.dont-lint-me.yaml\n'
+ '/bin/\n')
+ with open(os.path.join(self.wd, '.yamlignore'), 'w') as f:
+ f.write('!/bin/*.lint-me-anyway.yaml\n')
+
sys.stdout = StringIO()
with self.assertRaises(SystemExit):
cli.run(('-f', 'parsable', '.'))
--- a/yamllint/config.py
+++ b/yamllint/config.py
@@ -103,14 +103,15 @@ class YamlLintConfig:
'ignore and ignore-from-file keys cannot be used together'
)
elif 'ignore-from-file' in conf:
- if not isinstance(conf['ignore-from-file'], (str, list)):
+ if isinstance(conf['ignore-from-file'], str):
+ conf['ignore-from-file'] = [conf['ignore-from-file']]
+ if not (isinstance(conf['ignore-from-file'], list) and all(
+ isinstance(ln, str) for ln in conf['ignore-from-file'])):
raise YamlLintConfigError(
'invalid config: '
'ignore-from-file should contain filename(s)'
', either as a list or string'
)
- if isinstance(conf['ignore-from-file'], str):
- conf['ignore-from-file'] = [conf['ignore-from-file']]
with fileinput.input(conf['ignore-from-file']) as f:
self.ignore = pathspec.PathSpec.from_lines('gitwildmatch', f)
elif 'ignore' in conf:@ndrwnaguib I allowed myself to push on your branch. If you are OK with my changes (commit 2f8ad70), I'll merge.
yamllint/config.py
Outdated
| if not isinstance(conf['ignore-from-file'], (str, list)): | ||
| raise YamlLintConfigError( | ||
| 'invalid config: ' | ||
| 'ignore-from-file should contain filename(s)' | ||
| ', either as a list or string' | ||
| ) |
There was a problem hiding this comment.
This doesn't check that the subitems are strings!
So yamllint would stall with a misconfiguration like:
extends: default
ignore-from-file: [0]
tests/test_config.py
Outdated
| './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, | ||
| ))) |
There was a problem hiding this comment.
There are multiple things to enhance in these tests, for instance there is no control test (without ignore nor ignore-from-file) on the same data, and these similar tests would benefit from being gathered in the same class (to avoid reconstructing it each time).
|
I don't know why GitHub marks this PR as closed after I pushed to your branch: git remote add ndrwnaguib git@github.com:ndrwnaguib/yamllint.git
git push -fu ndrwnaguib masterI guess it's because your PR branch is named Anyway, tell me if the commit 2f8ad70 is OK for you. If it is, I'll push it to my |
that's odd, I am not sure, but yeah using a descriptive branch name could have been better anyways.
yes, thanks for the changes. |
|
Merged! |
ignore-from-filekey option.