enhancement: add rules plugin support #315

ssato · 2020-09-07T16:41:33Z

Add plugin support using setuptools (pkg_resources) plugin mechanism to
YAML Lint to allow users to add their own custom lint rule plugins.

Signed-off-by: Satoru SATOH satoru.satoh@gmail.com

ssato · 2020-09-07T16:43:22Z

Here is a working YAML Lint plugin example tested with it: https://github.com/ssato/yamllint-plugin-example

coveralls · 2020-09-07T16:45:38Z

Coverage decreased (-0.03%) to 97.477% when pulling b21eb6c on ssato:feature/plugin into 85ccd62 on adrienverge:master.

adrienverge · 2020-09-07T18:24:33Z

Interesting! Supporting plugins raise new issues (like security), for info please look this similar pull request from 2016: #19.

Could you give more details about your use-case? What kind of rules would you like to plug in? Wouldn't they be better inside yamllint, upstream?

ssato · 2020-09-08T00:34:36Z

Interesting! Supporting plugins raise new issues (like security), for info please look this similar pull request from 2016: #19.

Unlike #19, my plugin code does not use hidden dir to put custom rules nor modify sys.path, and plugins must be installed as usual python modules, AFAIK. So I don't think it brings huge security risks. Also, there might be security risk if users change $PYTHONPATH but it should be another story, I guess.

Could you give more details about your use-case? What kind of rules would you like to plug in? Wouldn't they be better inside yamllint, upstream?

There are cases that users want their own unique rules never come into yamllint, I think. For example, a rule forbids that some words are not allowed as mapping objects' keys.

adrienverge · 2020-09-08T11:18:58Z

@ssato I agree that the python module idea is good 👍

Supporting plugins in yamllint would also require adding documentation and tests. Also, I would prefer dropping the override parameter, and don't allow to re-define an existing rule, to make things clear and less error-prone for users. What do you think?

ssato · 2020-09-08T13:15:30Z

@ssato I agree that the python module idea is good +1

Supporting plugins in yamllint would also require adding documentation and tests. Also, I would prefer dropping the override parameter, and don't allow to re-define an existing rule, to make things clear and less error-prone for users. What do you think?

I changed my mind and fully agree with you to drop that part.
I'll try update it w/o 'override' part asap. Could you please wait for a minutes?

And about doc and tests, I'll try a little more.

ssato · 2020-09-09T10:14:12Z

A couple of updates:

doc: I pushed the change to docs/development.rst to explain how to develop rule plugins as a starter. English is not my native language and it should need fixes and updates but better than nothing, I think.
add runtime dependency to setuptools

ssato · 2020-09-09T18:18:10Z

Update:

I squashed and pushed them as two commits because adding plugin test case feels a bit harder and should take some more time. I cannot think of clean solution for this issue now.

10467a4: add plugin support and update doc
ef3c797: add a function test case of plugin support

adrienverge

Thanks @ssato. I have a few remarks, please see in the code.

I squashed and pushed them as two commits because adding plugin test case feels a bit harder and should take some more time. I cannot think of clean solution for this issue now.

Once merged into yamllint, the plugin feature will be used and will need some work to maintain, and some support on the GitHub issues page. Therefore this feature will be accepted if the code is perfect and comes with tests (and in my opinion one commit is enough since it's all the same functionnality).

yamllint/plugins.py

adrienverge · 2020-09-15T07:02:06Z

yamllint/plugins.py

+            rules_mod = entry.load()
+            yield rules_mod.RULES_MAP
+        except AttributeError:
+            pass  # TBD


What do you mean with TBD?

What do you think about removing load_plugin_rules_itr() and loading plugins into update_rules_map(), without try/except? If importing a plugin fails → no silent error. The problem is clearly visible in the stacktrace and the user knows why the custom plugin doesn't work, and how to fix it (uninstall the plugin or fix it).

TBD means to be determined to fix what to do if plugin is broken or has some problems to work.
I have some ideas about it, e.g. output warning messages about broken plugins, but I'm not sure twhat is the best to do for such cases you think so that I commented as it is.

I think that maybe there are cases that the quality of plugin code is not enough compared to yamllint itself you keep maintained and it would be better to keep yamllint works even if such plugins are installed.

adrienverge · 2020-09-15T07:12:50Z

yamllint/rules/__init__.py

@@ -14,6 +14,8 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.

+import yamllint.plugins
+


This space is not needed I think, yamllint.* import should be grouped.

Please let me clarify that your thought. Do you think that it should be something like:

from yamllint import ( plugins, rules.braces, ... )

or anything else?

I fully agree with you about that the empty line 18 is not needed and pushed that change already (commit ad1f292) but I'm not sure to group those (plugin and rules.*) imports is good thing from your maintainer's point of view (I think of the cases that addition and/or removal of standard rules).

adrienverge · 2020-09-15T07:17:14Z

yamllint/rules/__init__.py

@@ -63,8 +65,11 @@
 }


-def get(id):
-    if id not in _RULES:
+def get(id, rules=None):


If nobody uses the optional parameter rules=None, I'm for removing it.

I'm not sure but it might be better to keep it as is or allow passing some arguments to make easier for unit tests, I think.

To make tests easier and leave a room to control the rules' mappings to search for, those are necessary, I think.Please take a look at the commit 01554f2 also.

adrienverge · 2020-09-15T07:23:10Z

yamllint/rules/__init__.py

+    if rules is None:
+        rules = yamllint.plugins.update_rules_map(_RULES)


I see 2 issues:

It appears that this call modifies the _RULES object.

This get() function is called many times by the linter, it would be better not to call yamllint.plugins.update_rules_map() every time.

What do you think of:

_EXTERNAL_RULES = None def get(id): global _RULES, _EXTERNAL_RULES if _EXTERNAL_RULES is None: _EXTERNAL_RULES = yamllint.plugins.load_plugin_rules() if id in _RULES: return _RULES[id] elif id in _EXTERNAL_RULES: return _EXTERNAL_RULES[id] else: raise ValueError('no such rule: "%s"' % id)

?

Absolutely right! I like the code looks best and don't have any objections about it.

One thing I forgot to mention.

How about to add arguments {rules=None, external_rules=None} (both will fallback to_RULES, _EXTERNAL_RULES if no arguments were given) to 'get' function to make it easy to test?

Could you please take a look at the commit 01554f2 ? I think it got better.

ssato · 2020-09-17T14:53:34Z

As far as I'm aware of, the following issues have to be fixed and/or improved before merge.

Decide what to do for cases if something goes wrong with loaded plugin: https://github.com/adrienverge/yamllint/pull/315/files#diff-6fb0badc7fe341843582e3b9789c78d2R50
- I pushed an experimental commit to improve this. @adrienverge How do you think this change cb7ae13 for this issue?
Fix/improve yamllint.rules.get called many times: https://github.com/adrienverge/yamllint/pull/315/files#diff-85022b669c894366f26191332b108dd5R69
- I pushed another experimental commit 3503eb0 for this. It introduces another runtime dependency to functools32 for python 2.7 and can help little for the issue unfortunately.
Cleanup the imports in yamllint.rules: https://github.com/adrienverge/yamllint/pull/315/files#diff-85022b669c894366f26191332b108dd5R17

Could you please point out if I forgot to list?

And other relatively minor things may be followings, I think.

Keep coverage rate.
The real plugin test cases using pkg_resources actually. I think that another plugin repo might be needed for this.
Doc needs updates. I added a section about the plugin but it should need fixes and improvements.
Perhaps, it might be better to change the package group name to "yamllint.plugins.rules" or something.

ssato · 2020-09-23T04:33:28Z

* [ ]  Fix/improve yamllint.rules.get called many times: https://github.com/adrienverge/yamllint/pull/315/files#diff-85022b669c894366f26191332b108dd5R69
  
  * I pushed another experimental commit [3503eb0](https://github.com/adrienverge/yamllint/commit/3503eb0ca36117e39f455f8502e0da988e69171e) for this. It introduces another runtime dependency to functools32 for python 2.7 and can help little for the issue unfortunately.

I reverted this change because it does not look improving its performance a lot as far as I tested. Sorry for the confusion.

$ pytest tests/test_get_benchmark.py
============================================================================= test session starts ==============================================================================
platform linux -- Python 3.8.5, pytest-4.6.11, py-1.8.2, pluggy-0.13.1
benchmark: 3.2.2 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /home/ssato/repos/public/github.com/ssato/yamllint.git
plugins: forked-1.1.1, xdist-1.31.0, shutil-1.7.0, virtualenv-1.7.0, benchmark-3.2.2, cov-2.10.1, testinfra-5.3.1, mock-1.10.4
collected 4 items                                                                                                                                                              

tests/test_get_benchmark.py ....                                                                                                                                         [100%]


---------------------------------------------------------------------------------------- benchmark: 4 tests ---------------------------------------------------------------------------------------
Name (time in ms)                       Min                Max               Mean            StdDev             Median               IQR            Outliers      OPS            Rounds  Iterations
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
test_get_memoized_benchmark         16.3170 (1.0)      27.9499 (1.0)      18.7482 (1.0)      2.9772 (1.06)     17.9337 (1.0)      0.5738 (1.10)          6;9  53.3386 (1.0)          55           1
test_get_benchmark                  32.8898 (2.02)     45.5125 (1.63)     35.1427 (1.87)     3.0465 (1.08)     34.1384 (1.90)     0.8731 (1.67)          4;4  28.4554 (0.53)         30           1
test_get_from_globals_benchmark     36.0053 (2.21)     46.9512 (1.68)     38.0426 (2.03)     2.8079 (1.0)      37.1185 (2.07)     0.5223 (1.0)           3;5  26.2863 (0.49)         27           1
test_get_2_benchmark                44.4627 (2.72)     56.1275 (2.01)     47.0150 (2.51)     3.3407 (1.19)     45.9103 (2.56)     0.7719 (1.48)          3;3  21.2698 (0.40)         22           1
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Legend:
  Outliers: 1 Standard Deviation from Mean; 1.5 IQR (InterQuartile Range) from 1st Quartile and 3rd Quartile.
  OPS: Operations Per Second, computed as 1 / Mean
=========================================================================== 4 passed in 5.47 seconds ===========================================================================
$ cat tests/test_get_benchmark.py
import functools
import random

from yamllint.rules import _RULES, _PLUGIN_RULES


RULE_IDS = list(_RULES.keys()) + list(_PLUGIN_RULES.keys())
RULE_IDS = random.sample(RULE_IDS, len(RULE_IDS))


def get(rule_id, rules=None, plugin_rules=None):
    if rules is None:
        rules = _RULES

    if plugin_rules is None:
        plugin_rules = _PLUGIN_RULES

    if rule_id in rules:
        return rules[rule_id]

    if rule_id in plugin_rules:
        return plugin_rules[rule_id]

    raise ValueError('no such rule: "%s"' % rule_id)


def get_2(rule_id, rules=None, plugin_rules=None):
    if rules is None:
        rules = _RULES

    if plugin_rules is None:
        plugin_rules = _PLUGIN_RULES

    rule = rules.get(rule_id, plugin_rules.get(rule_id, None))
    if rule is None:
        raise ValueError('no such rule: "%s"' % rule_id)

    return rule


def get_from_globals(rule_id):
    rule = _RULES.get(rule_id, _PLUGIN_RULES.get(rule_id, None))
    if rule is None:
        raise ValueError('no such rule: "%s"' % rule_id)

    return rule


@functools.lru_cache()
def memoized_get(rule_id):
    return get_2(rule_id)


def run_gets(fun, times=10000):
    return [[fun(rid) for rid in RULE_IDS] for _idx in range(times)]


def test_get_benchmark(benchmark):
    assert benchmark(run_gets, get)


def test_get_2_benchmark(benchmark):
    assert benchmark(run_gets, get_2)


def test_get_from_globals_benchmark(benchmark):
    assert benchmark(run_gets, get_from_globals)


def test_get_memoized_benchmark(benchmark):
    assert benchmark(run_gets, memoized_get)
$

adrienverge

Hello @ssato,

Decide what to do for cases if something goes wrong with loaded plugin:

I'm OK with your proposal (a warning).

Decide what to do for cases if something goes wrong with loaded plugin:

Using a LRU cache for this is probably overkill and complexifies the code. What about my original suggestion: just build an _EXTERNAL_RULES in parallel of _RULES? EDIT: I see in you finally reverted back to it.

How about to add arguments {rules=None, external_rules=None}

If they are not used in real code (not tests), they shouldn't be. Modifying code just for tests is usually a bad practice.
If you need to test different values of _RULES and _EXTERNAL_RULES, you can patch them using a mock?

The real plugin test cases using pkg_resources actually. I think that another plugin repo might be needed for this.

It would be complicated; in my opinion your example in tests is good 👍

Perhaps, it might be better to change the package group name to "yamllint.plugins.rules" or something.

Good idea! yamllint.plugins.rules sound good.

adrienverge · 2020-09-23T08:25:42Z

tests/plugins/example/__init__.py

+#
+# SPDX-License-Identifier: GPL-3.0-or-later
+#
+"""YAML Lint plugin entry point


The tool is named "yamllint", not "YAML Lint".

Thanks for your correction! Applied the fix and pushed it: 8253b1c

adrienverge · 2020-09-23T08:26:04Z

tests/plugins/example/__init__.py

+#
+# Copyright (C) 2020 Satoru SATOH
+#
+# SPDX-License-Identifier: GPL-3.0-or-later


Can you please stay consistent with other license headers, even in the plugin example code?

Same as above.

ssato

Hello @adrienverge,

Thanks a lot for your feedbacks!
I think that I applied all changes you advised and could you please look at them in the following comments?

If you're OK with them, I'll squash all into a few commits or a commit for further review.

Decide what to do for cases if something goes wrong with loaded plugin:

I'm OK with your proposal (a warning).

Thanks! Then, I keep as it is.

Decide what to do for cases if something goes wrong with loaded plugin:

Using a LRU cache for this is probably overkill and complexifies the code. What about my original suggestion: just build an _EXTERNAL_RULES in parallel of _RULES? EDIT: I see in you finally reverted back to it.

How about to add arguments {rules=None, external_rules=None}

If they are not used in real code (not tests), they shouldn't be. Modifying code just for tests is usually a bad practice.
If you need to test different values of _RULES and _EXTERNAL_RULES, you can patch them using a mock?

The real plugin test cases using pkg_resources actually. I think that another plugin repo might be needed for this.

It would be complicated; in my opinion your example in tests is good +1

Absolutely right. I was not sure mock works for such cases but it looks so.
I changed as you advised and modified its test code in 507d824 and 44a3559.
Could you please take a look at these changes?

Perhaps, it might be better to change the package group name to "yamllint.plugins.rules" or something.

Good idea! yamllint.plugins.rules sound good.

Thanks! I changed so in 081fee3 and 33d44f2.

adrienverge

Thanks @ssato, here are some new comments. I need to take time to test the code and review it in detail.

adrienverge · 2020-09-26T09:15:41Z

.travis.yml

@@ -13,6 +13,7 @@ env:
  - REMOVE_LOCALES=true
 install:
  - pip install pyyaml coveralls flake8 flake8-import-order doc8
+  - if [[ $TRAVIS_PYTHON_VERSION != 3* ]]; then pip install mock; fi


Python 2 support and all related code will be dropped soon. So can we avoid this line, and don't execute your new tests if Python < 3? Maybe with something like @unittest.skipIf(sys.version_info < (3, 0)... or equivalent?

I understand. I don't have strong opinion about that and pushed such changes in c9af954 and 8040230 just in case.

adrienverge · 2020-09-26T09:16:25Z

docs/development.rst

+
+Yamllint rule plugins must satisfy the followings.
+
+#. It must be a python package installable using pip and distributed under


python → Python

Thanks! I applied changes in 8864b19.

adrienverge · 2020-09-26T09:17:21Z

docs/development.rst

+#. It must contains the entry point configuration in setup.cfg or something
+   similar packaging configuration files, to make it installed and working as a
+   yamllint plugin like below. (<plugin_name> is that plugin name and
+   <plugin_src_dir> is a dir where the rule modules exist.)


Suggested change

#. It must contains the entry point configuration in setup.cfg or something

similar packaging configuration files, to make it installed and working as a

yamllint plugin like below. (<plugin_name> is that plugin name and

<plugin_src_dir> is a dir where the rule modules exist.)

#. It must contains the entry point configuration in ``setup.cfg`` or something

similar packaging configuration files, to make it installed and working as a

yamllint plugin like below. (``<plugin_name>`` is that plugin name and

``<plugin_src_dir>`` is a dir where the rule modules exist.)

Same as the previous one.

adrienverge · 2020-09-26T09:20:31Z

tests/plugins/example/override_comments.py

@@ -0,0 +1,51 @@
+#
+# Copyright (C) 2020 Satoru SATOH
+# SPDX-License-Identifier: GPL-3.0-or-later


Can you please stay consistent with other license headers, even in the plugin example code?

Thanks! I forgot to fix that. Fixed in b49e67f.

adrienverge · 2020-09-26T09:24:07Z

yamllint/plugins.py

+def validate_rule_module(rule_mod):
+    """Test if given rule module is valid.
+    """
+    return getattr(rule_mod, "ID", False
+                   ) and callable(getattr(rule_mod, "check", False))


Do you think it would be a good idea to check if TYPE, CONF and DEFAULT are also present?

Some rules don't have CONF and DEFAULT like yamllint/rules/comments_indentation.py.
So how about to check if both ID and TYPE are present? I pushed such change in ec9b177 just in case.

Sure, you're totally right.

ssato · 2020-10-01T18:53:12Z

@adrienverge Are there any issues left behind or I forgot? If it's OK, I'll squash all of them into a few commits to prepare you can easily merge them.

adrienverge · 2020-10-01T19:12:03Z

I don't know about issues, I still need to take time to test and review. But you can squash 👍

ssato · 2020-10-01T20:06:52Z

I don't know about issues, I still need to take time to test and review. But you can squash +1

Thanks! I squashed them into 3 commits.

adrienverge

Hello @ssato, thanks for the work! Here is a first review.

The first thing I have to say is that it's not obvious how to make a plugin, or how to enable it. Hence, I propose some modifications.

Simplify docs/development.rst a lot:

Develop rule plugins
--------------------

yamllint provides a plugin mechanism using setuptools (pkg_resources) to allow
adding custom rules. So, you can extend yamllint and add rules with your own
custom yamllint rule plugins if you developed them.

yamllint plugins are Python packages installable using pip and distributed under
GPLv3+. To develop yamllint rules, it is recommended to copy the example from
``tests/plugins/yamllint_plugin_example``, and follow its README file. Also, the
core rules themselves in ``yamllint/rules`` are good references.

Write most of the rules how-to inside tests/plugins/yamllint_plugin_example/README.rst.

Name the example yamllint_plugin_example (not just example) and make it a real distributable example. This way, any user would easily know where to start.

For this, it should contain:

A README.rst file, that explains what you did previously, and how to install the plugin using pip install, etc.
setup.py:
```
import setuptools
setuptools.setup()
```

setup.cfg:

[metadata]
name = yamllint_plugin_example
version = 1.0.0

[options]
packages = find:
install_requires = yamllint

[options.entry_points]
yamllint.plugins.rules =
    example = yamllint_plugin_example

For completeness, I propose to add a file tests/plugins/yamllint_plugin_example/yamllint_plugin_example/random_failure.py:

# -*- coding: utf-8 -*-
# Copyright (C) 2020 Adrien Vergé
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program.  If not, see <http://www.gnu.org/licenses/>.

import random

from yamllint.linter import LintProblem

ID = 'random-failure'
TYPE = 'token'

def check(conf, token, prev, next, nextnext, context):
    if random.random() > 0.9:
        yield LintProblem(token.start_mark.line + 1,
                          token.start_mark.column + 1,
                          'random failure')

Idea: rename override-comments to forbid-comments?
Inside the plugin, change RULES_MAP = { to RULES = {, for simplicity.

What do you think?

ssato · 2020-10-04T04:57:27Z

Hello @ssato, thanks for the work! Here is a first review.

Thanks for the comments! @adrienverge.

Before going into details, I think that it should be better to have a standalone git repo separate from yamllint's repo as a rules plugin reference implementation fully tested, git-{clone,fork}-able, and provides the followings.

rules' code works and fully tested
tests and CI data; test code, tox.ini which is necessary for a kind of integration tests, I think, ci configuration examples, and so son.
packaging data; setup.py, setup.cfg (pyproject.toml in the future) and so on.

So, I made changes as you said basically but those might be a temporal solution.

The first thing I have to say is that it's not obvious how to make a plugin, or how to enable it. Hence, I propose some modifications.

* Simplify `docs/development.rst` a lot:
  ```
  Develop rule plugins
  --------------------
  
  yamllint provides a plugin mechanism using setuptools (pkg_resources) to allow
  adding custom rules. So, you can extend yamllint and add rules with your own
  custom yamllint rule plugins if you developed them.
  
  yamllint plugins are Python packages installable using pip and distributed under
  GPLv3+. To develop yamllint rules, it is recommended to copy the example from
  ``tests/plugins/yamllint_plugin_example``, and follow its README file. Also, the
  core rules themselves in ``yamllint/rules`` are good references.
  ```
* Write most of the rules how-to inside `tests/plugins/yamllint_plugin_example/README.rst`.

I agree, and changed docs/development.rst as you said and add tests/yamllint_plugin_example/README.rst to provide detailed information.

* Name the example `yamllint_plugin_example` (not just `example`) and make it a real distributable example. This way, any user would easily know where to start.

I just renamed from tests/plugin to tests/yamllint_plugin_example and from tests/plugin/example/ to tests/yamllint_plugin_rules to run tests easier, add setup.py.example because the file named setup.py causes tests error, setup.cfg and README.rst I mentioned before.

  * For completeness, I propose to add a file `tests/plugins/yamllint_plugin_example/yamllint_plugin_example/random_failure.py`:

Thanks! I like it and added.

* Idea: rename `override-comments` to `forbid-comments`?

I agree and renamed it.

* Inside the plugin, change `RULES_MAP = {` to `RULES = {`, for simplicity.

I agree. I changed RULES_MAP to RULES and simplified it because it's not necessary to be a dict.

adrienverge

Thanks @ssato, it's starting to look good!

Please see my remarks below, I fixed some of them + other things in commit c43a149 that I pushed directly on your branch (feel free to squash if you want).

I think that it should be better to have a standalone git repo separate from yamllint's repo as a rules plugin reference implementation fully tested, git-{clone,fork}-able, and provides the followings.

I get your point, but in my opinion it's needed to have the plugin example in the same repo, for some of the same reasons that you describe: being able to test both:
- the plugins,
- the yamllint code that uses plugins.

By the way you renamed setup.py to setup.py.example because "it caused tests error": what errors do you encounter? On my computer, using the normal filename setup.py works fine, and python -m unittest discover passes.

Most importantly, this feature lacks tests. In test_plugins.py we need tests that simulate a yamllint with plugins (for example the ones in tests/yamllint_plugin_example/rules), and run the linter on fake files, and makes sure everything is reported as expected.
This way, future bugs or regressions on the plugin code will be noticed.
For this purpose, maybe it would help to add a 3rd fake rule (very simple, again)? An idea: rule no-forty-two, that would error when the number 42 is found as a yaml.ValueToken? It would be easy to unit-test.
Again: making code more complex (adding arguments to load_plugin_rules_itr(entry_points=None, group=PACKAGE_GROUP)) just to be able to test it is a bad practice. We should use mocks instead.
I fixed this in my commit.

Good luck! Next time, it would be awesome if the code was perfect, so we can merge it!

adrienverge · 2020-10-08T06:45:52Z

tests/yamllint_plugin_example/rules/forbid_comments.py

+    """Check if comments are found.
+    """


I think these docstrings everywhere aren't needed, they say exactly what the file/function already says. To me it brings more confusion. These examples should instead be short and simple.

Thanks! I rebased it to follow changes in master branch, commit and pushed 0588797 for this.

adrienverge · 2020-10-08T07:20:36Z

tests/test_plugins.py

+            self.assertFalse(fun(rule_mod))
+
+    @unittest.skipIf(not mock, "unittest.mock is not available")
+    @mock.patch('pkg_resources.iter_entry_points')


Tests fail on Python 2.7 because of this change I just pushed (because mock == False on Python 2).

To solve it, I see at least 2 options:

Reintroduce your code in .travis.yml (from enhancement: add rules plugin support #315 (comment)).

Use another way to call mock, for example:
def test_load_plugin_rules_itr(self): with mock.patch('pkg_resources.iter_entry_points') as iter_entry_points: iter_entry_points.return_value = []

I chosen 2 because Python 2.7 support might be stopped not too far ahead.

OK! As you prefer.

Indeed all Python 2 related code will be dropped in 3 months.

Thanks, I pushed ca6274e for this.

ssato · 2020-10-08T19:09:46Z

1. > I think that it should be better to have a standalone git repo separate from yamllint's repo as a rules plugin reference implementation fully tested, git-{clone,fork}-able, and provides the followings.
   
   
   I get your point, but in my opinion it's needed to have the plugin example in the same repo, for some of the same reasons that you describe: being able to test both:
   - the plugins,
   - the yamllint code that uses plugins.
   By the way you renamed `setup.py` to `setup.py.example` because "it caused tests error": what errors do you encounter? On my computer, using the normal filename `setup.py` works fine, and `python -m unittest discover` passes.
   Most importantly, this feature lacks tests. In `test_plugins.py` we need tests that simulate a yamllint with plugins (for example the ones in `tests/yamllint_plugin_example/rules`), and run the linter on fake files, and makes sure everything is reported as expected.
   This way, future bugs or regressions on the plugin code will be noticed.
   For this purpose, maybe it would help to add a 3rd fake rule (very simple, again)? An idea: rule `no-forty-two`, that would error when the number `42` is found as a `yaml.ValueToken`? It would be easy to unit-test.

Sorry to add another code needs review but I pushed 48462f5 for this issue that it lacks of plugin tests.
It's still not complete plugin tests but may improve some, I think.

adrienverge · 2020-10-12T11:58:02Z

Hello @ssato,

I think the main code (in yamllint/) is ready.

However I was talking about testing the yamllint code that uses plugins. Testing that the no-forty-two and forbid-comments rules work is moderately useful, but what is most useful is to check that yamllint correctly loads these rules, and correctly uses them.
Would you be OK to move all these new tests in tests/test_plugin.py? This way you can more easily add the new required tests that are still missing:

check what happens when plugins are loaded but disabled in conf,
check what happens when plugins are loaded but enabled in conf (= the ones you wrote in your last commit),
check that yamllint can run and lint a simple file when a plugin is bugged, and that yamllint produces a warning,
etc. (please help me imagining corner cases)

ssato · 2020-10-18T15:58:22Z

Hello @adrienverge,

Hello @ssato,

I think the main code (in yamllint/) is ready.

Thanks a lot!

However I was talking about testing the yamllint code that uses plugins. Testing that the no-forty-two and forbid-comments rules work is moderately useful, but what is most useful is to check that yamllint correctly loads these rules, and correctly uses them.
Would you be OK to move all these new tests in tests/test_plugin.py? This way you can more easily add the new required tests that are still missing:
* check what happens when plugins are loaded but disabled in conf,

* check what happens when plugins are loaded but enabled in conf (= the ones you wrote in your last commit),

* check that yamllint can run and lint a simple file when a plugin is bugged, and that yamllint produces a warning,

I'm not sure what you want exactly but added each plugin test cases in tests/yamllint_plugin_example/tests/ and some integration test cases run linter with disabled plugins, will be generated dynamically as same as tests/test_spec_examples.py does in tests/test_plugins.py.

adrienverge

Hello @ssato,

I'm talking about removing tests from tests/yamllint_plugin_example/tests/ (or leaving just very basic ones, just to encourage developers to write tests for their plugins) and moving all these tests to tests/test_plugins.py. What is most useful is to check that yamllint correctly loads these rules, and correctly uses them.

Please don't change tests/test_spec_examples.py because it's not meant for that.

adrienverge · 2020-10-12T11:47:52Z

tests/yamllint_plugin_example/tests/plugins_common.py

+        if not self.rule_id:
+            return


Suggested change

if not self.rule_id:

return

assert self.rule_id

if self.rule_id is not set, it's probably a problem and should be reported?

Thanks! Absolutely, right. Fixed in 007b9cf.

adrienverge · 2020-10-12T11:48:17Z

tests/yamllint_plugin_example/tests/test_forbid_comments.py

+from .plugins_common import (
+    mock, PluginTestCase
+)


Can you make this fit one line to match the project style?

(same at other places in this commit)

Fixed in f100ced.

adrienverge · 2020-10-12T11:50:15Z

tests/yamllint_plugin_example/tests/plugins_common.py

+    mock = False
+
+import yamllint.rules
+from .common import RuleTestCase


I don't like the idea of copying test/common.py to tests/yamllint_plugin_example/tests/common.py, with the exact same content. We would have to maintain two identic files, which is cumbersome for multiple reasons.

I propose to remove tests/yamllint_plugin_example/tests/common.py, and use:

Suggested change

from .common import RuleTestCase

from ...common import RuleTestCase

I did so because I want that plugin developers can devel their plugins by copying tests/yamllint_plugin_example with least modifications but I understand that and removed the copy.

adrienverge · 2020-10-12T11:51:16Z

tests/yamllint_plugin_example/tests/plugins_common.py

+from ..rules import RULES
+
+
+PLUGIN_RULES = dict(RULES)


Why this line? Why not just using RULES on line 39?

Thanks! Also absolutely, right. Fixed in 09e6530.

adrienverge · 2020-10-12T11:53:42Z

tests/test_plugins.py

+import yamllint.rules
+
+
+PLUGIN_RULES = dict(example.RULES)


(same as previous comment)
Why this line? Why not just using example.RULES on lines 104-117?

Same as the above.

ssato · 2020-10-31T02:26:03Z

Please don't change tests/test_spec_examples.py because it's not meant for that.

I think that change should not change its original behavior.

And I did so to achieve the following. I can't think of any good ideas other than that now.

check that yamllint can run and lint a simple file when a plugin is bugged, and that yamllint produces a warning,

ssato · 2020-12-31T11:49:27Z

I'm talking about removing tests from tests/yamllint_plugin_example/tests/ (or leaving just very basic ones, just to encourage developers to write tests for their plugins) and moving all these tests to tests/test_plugins.py. What is most useful is to check that yamllint correctly loads these rules, and correctly uses them.

I did like you said. Could you please check the commit c377e63?

Please don't change tests/test_spec_examples.py because it's not meant for that.

I see and reverted the change in the commit ab2eb97.

adrienverge · 2021-01-11T15:48:20Z

Hello @ssato, thanks again for your work.
This PR takes much time to review, and it's the 4th or 5th review. Before I look to it again, can you make sure it's 100% ready?

Add plugin support using setuptools (pkg_resources) plugin mechanism to yamllint to allow users to add their own custom lint rule plugins. Also add some plugin support test cases, an example plugin as a reference, and doc section about how to develop rules' plugins. Signed-off-by: Satoru SATOH <satoru.satoh@gmail.com> Co-authored-by: Adrien Vergé

ssato · 2021-04-29T03:50:34Z

Very sorry to left it and gave no response to you for a long time. A lot happened in my personal life and could not work on this.

Anyway, I self-reviewed this again and have confident it works well and resolved all of the issues you kindly pointed out AFAIK.
I rebased the branch and squashed the commits into a commit.

perseoGI · 2024-09-26T15:29:38Z

Hi there!
Any update on this?
Is it possible to extend somehow yamllint to create custom rules?

adrienverge · 2024-10-01T18:42:52Z

Hello @perseoGI, no, it's not possible to extend yamllint to create custom rules.

Although I thank Satoru for trying with this pull request in 2020, I now think such addition is a bad idea. Plugin support would require an immutable API and much work to maintain. It's preferable to upstream rules that make sense (those about the form of YAML, not the content) inside the yamllint project itself.

perseoGI · 2024-10-02T11:48:04Z

@adrienverge understood, thank you for your response!

ssato force-pushed the feature/plugin branch from afa0496 to 3074488 Compare September 9, 2020 03:49

ssato force-pushed the feature/plugin branch from 9cf8903 to 10467a4 Compare September 9, 2020 10:33

adrienverge reviewed Sep 15, 2020

View reviewed changes

ssato force-pushed the feature/plugin branch from ad1f292 to af8d060 Compare September 16, 2020 12:14

ssato force-pushed the feature/plugin branch from 3503eb0 to cb7ae13 Compare September 23, 2020 04:26

adrienverge reviewed Sep 23, 2020

View reviewed changes

ssato commented Sep 23, 2020

View reviewed changes

adrienverge reviewed Sep 26, 2020

View reviewed changes

ssato force-pushed the feature/plugin branch from 44a3559 to 897594a Compare September 26, 2020 15:20

ssato force-pushed the feature/plugin branch 3 times, most recently from 6ae0956 to 9f9e282 Compare October 1, 2020 20:04

adrienverge reviewed Oct 2, 2020

View reviewed changes

ssato force-pushed the feature/plugin branch from 7aaa8e4 to 798f653 Compare October 4, 2020 04:36

adrienverge added a commit to ssato/yamllint that referenced this pull request Oct 8, 2020

Enhancements for adrienverge#315

c43a149

adrienverge reviewed Oct 8, 2020

View reviewed changes

ssato pushed a commit to ssato/yamllint that referenced this pull request Oct 8, 2020

Enhancements for adrienverge#315

6a1fead

ssato force-pushed the feature/plugin branch from e2d4114 to 48462f5 Compare October 8, 2020 18:26

ssato pushed a commit to ssato/yamllint that referenced this pull request Oct 18, 2020

Enhancements for adrienverge#315

10eb9b4

ssato force-pushed the feature/plugin branch from a282350 to f1eaed9 Compare October 18, 2020 02:13

adrienverge reviewed Oct 23, 2020

View reviewed changes

ssato force-pushed the feature/plugin branch from b81be25 to 79626a3 Compare December 31, 2020 07:44

ssato force-pushed the feature/plugin branch from fcefe04 to b21eb6c Compare April 29, 2021 03:41

		if rules is None:
		rules = yamllint.plugins.update_rules_map(_RULES)


		Yamllint rule plugins must satisfy the followings.

		#. It must be a python package installable using pip and distributed under

	from .common import RuleTestCase
	from ...common import RuleTestCase

enhancement: add rules plugin support #315

Are you sure you want to change the base?

enhancement: add rules plugin support #315

Conversation

ssato commented Sep 7, 2020

ssato commented Sep 7, 2020

coveralls commented Sep 7, 2020 • edited Loading

adrienverge commented Sep 7, 2020

ssato commented Sep 8, 2020

adrienverge commented Sep 8, 2020

ssato commented Sep 8, 2020 • edited Loading

ssato commented Sep 9, 2020

ssato commented Sep 9, 2020

adrienverge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssato Sep 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssato Sep 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssato commented Sep 17, 2020 • edited Loading

ssato commented Sep 23, 2020

adrienverge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssato left a comment • edited Loading

Choose a reason for hiding this comment

adrienverge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssato commented Oct 1, 2020

adrienverge commented Oct 1, 2020

ssato commented Oct 1, 2020

adrienverge left a comment • edited Loading

Choose a reason for hiding this comment

ssato commented Oct 4, 2020

adrienverge left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssato commented Oct 8, 2020

adrienverge commented Oct 12, 2020

ssato commented Oct 18, 2020

adrienverge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 7, 2020 •

edited

Loading

ssato commented Sep 8, 2020 •

edited

Loading

ssato Sep 15, 2020 •

edited

Loading

ssato Sep 15, 2020 •

edited

Loading

ssato commented Sep 17, 2020 •

edited

Loading

ssato left a comment •

edited

Loading

adrienverge left a comment •

edited

Loading

adrienverge left a comment •

edited

Loading

ssato commented Dec 31, 2020 •

edited

Loading