Improve docs clarity on loading custom checkers #7276

daogilvie · 2022-08-08T14:24:06Z

Type of Changes

	Type
✓	📜 Docs

Description

Short

This PR just updates the documentation page on writing custom checkers. It adds
information about some restrictions between how init-hook changes the loading
path and when you can use that to import your custom module.

Long (in the commit message)

Whilst the docs on testing custom checkers do touch on the fact the
module has to be in your load path, there are some restrictions that
were not made clear before.
Specifically, the init-hook configuration item (which is often used
explicitly to change the sys.path) is not automatically sufficient. If
the init hook is in a file, but the load-plugins argument is passed in
the CLI, then the plugins are loaded before the init-hook has run, and
so will not be imported. In this case, the failure can also be silent,
so the outcome is a lint that will simply not contain any information
from the checker, as opposed to a run that will error.
This corner case may merit fixing in a separate PR, that is to be
confirmed.

Closes #7264

Whilst the docs on testing custom checkers do touch on the fact the module has to be in your load path, there are some restrictions that were not made clear before. Specifically, the init-hook configuration item (which is often used explicitly to change the sys.path) is not automatically sufficient. If the init hook is in a file, but the load-plugins argument is passed in the CLI, then the plugins are loaded before the init-hook has run, and so will not be imported. In this case, the failure can also be silent, so the outcome is a lint that will simply not contain any information from the checker, as opposed to a run that will error. This corner case may merit fixing in a separate PR, that is to be confirmed. Closes #7264

DanielNoord

Could you start this note with something like:
The preferred way of making plugins available is by installing them as a package, etc.?

DanielNoord · 2022-08-08T14:26:35Z

doc/development_guide/how_tos/custom_checkers.rst

@@ -223,6 +223,18 @@ Now we can debug our checker!
    environment variable or by adding the ``my_plugin.py``
    file to the ``pylint/checkers`` directory if running from source.

+    If your pylint config has an init-hook that modifies
+    ``sys.path`` to include the module's parent directory, this
+    will also work, but only if:


Suggested change

will also work, but only if:

will also work, but only if either:

Just saw this myself, mortified, good spot thanks @DanielNoord!

@DanielNoord

Thanks @DanielNoord for pointing it out.

@DanielNoord

As per suggestion from @DanielNoord, this makes it clearer that installing the custom module is the recommended way.

daogilvie · 2022-08-08T14:51:34Z

@DanielNoord thank your for such a quick review! I've updated as per your suggestions, what do you think?

coveralls · 2022-08-08T15:58:42Z

Pull Request Test Coverage Report for Build 3008066091

0 of 0 changed or added relevant lines in 0 files are covered.
215 unchanged lines in 18 files lost coverage.
Overall coverage increased (+0.08%) to 95.333%

Files with Coverage Reduction	New Missed Lines	%
pylint/checkers/modified_iterating_checker.py	1	97.7%
pylint/testutils/_primer/primer_command.py	1	95.65%
pylint/epylint.py	2	83.33%
pylint/extensions/docparams.py	2	98.53%
pylint/testutils/_primer/primer_compare_command.py	2	96.43%
pylint/utils/file_state.py	2	95.41%
pylint/lint/expand_modules.py	3	94.74%
pylint/testutils/_primer/primer_prepare_command.py	5	21.43%
pylint/checkers/classes/special_methods_checker.py	8	94.97%
pylint/config/find_default_config_files.py	8	89.16%

Totals
Change from base Build 2819506531:	0.08%
Covered Lines:	17017
Relevant Lines:	17850

💛 - Coveralls

DanielNoord

Let's wait on the discussion in the issue before merging this. L229-L240 might need to change.

Rest looks good to me! Thanks @daogilvie 😄

doc/whatsnew/fragments/7264.other

doc/development_guide/how_tos/custom_checkers.rst

daogilvie · 2022-08-08T16:59:23Z

No problem! Thank you for very patiently reviewing my first attempt at a documentation PR 😂 I have some bad habits in terms of brevity and language to unlearn.

I'll make the changes above and then, as you say, pause this until after any potential changes in behaviour.

@DanielNoord

Thanks to @DanielNoord Co-authored-by: Daniël van Noord <[email protected]>

This is not the way. Co-authored-by: Daniël van Noord <[email protected]>

Co-authored-by: Daniël van Noord <[email protected]>

This is a much clearer way of explaining what is meant by this recommendation. Co-authored-by: Daniël van Noord <[email protected]>

daogilvie · 2022-08-08T17:14:17Z

Side note: I have made the commits individually in the UI for convenience, as your suggestions made sense to me straight away. Is that something you as maintainers prefer or dislike? Would it be better to have one commit for all recommendations? I didn't seem to have the "add to batch" button available.

DanielNoord · 2022-08-08T18:35:14Z

Side note: I have made the commits individually in the UI for convenience, as your suggestions made sense to me straight away. Is that something you as maintainers prefer or dislike?

Usually I/We prefer one commit, but it doesn't matter too much. As long as you don't rebase or force push the Github UI is pretty good at showing what changed and what remained the same after new commits.

Would it be better to have one commit for all recommendations? I didn't seem to have the "add to batch" button available.

I know, very annoying. It only shows up in the files view which you can select at the top of PR. There you can add to batch and then do one single commit.

Pierre-Sassoulas

LGTM, thanks a lot !

doc/development_guide/how_tos/custom_checkers.rst

Pierre-Sassoulas · 2022-09-07T13:37:57Z

@daogilvie should we close this ? It seems it would be a valuable addition to the doc ?

@DanielNoord why do we have a blocked label ?

daogilvie · 2022-09-07T13:39:11Z

Oh goodness I completely forgot about this. It was blocked pending resolution of the other PR.

doc/development_guide/how_tos/custom_checkers.rst

DanielNoord · 2022-09-18T10:26:33Z

doc/development_guide/how_tos/custom_checkers.rst

+
+    * the ``init-hook`` and the ``load-plugins`` list are both
+      defined in a pylintrc file, or...
+    * the ``init-hook`` is passed as a command-line argument and


Let's explicitly state what doesn't work

doc/development_guide/how_tos/custom_checkers.rst

DanielNoord · 2022-09-18T10:27:14Z

doc/whatsnew/fragments/7264.other

@@ -0,0 +1,3 @@
+Dev documentation is now clearer about custom checker loading.


I don't think this needs a fragment. I'll add the skip news label.

daogilvie · 2022-09-19T16:16:14Z

I have just pushed a commit with the review fixes in, but I think my deletion of the previous fork has left this PR in a bad state. @DanielNoord @Pierre-Sassoulas I think I may have to close this PR and re-open a new one from the replacement fork for this to work. Do you mind?

Pierre-Sassoulas · 2022-09-19T17:53:13Z

Not at all, don't hesitate to ping me for a review :)

daogilvie · 2022-09-20T07:24:20Z

Replaced with #7502

daogilvie · 2022-09-20T07:25:25Z

Not at all, don't hesitate to ping me for a review :)

I don't seem to be able to do this directly (the reviewers section of the PR gives me no options), sorry.

Pierre-Sassoulas · 2022-09-20T15:03:46Z

You can use @Pierre-Sassoulas :)

DanielNoord reviewed Aug 8, 2022

View reviewed changes

daogilvie added 2 commits August 8, 2022 15:43

Edit for clarity as per review comment

779ba10

Thanks @DanielNoord for pointing it out.

Update wording to recommend installation

0951441

As per suggestion from @DanielNoord, this makes it clearer that installing the custom module is the recommended way.

DanielNoord requested changes Aug 8, 2022

View reviewed changes

daogilvie and others added 5 commits August 8, 2022 18:00

Better wording in towncrier fragment

7e53ea5

Thanks to @DanielNoord Co-authored-by: Daniël van Noord <[email protected]>

Remove suggestion to edit pylint source

52d6ebd

This is not the way. Co-authored-by: Daniël van Noord <[email protected]>

Improve clarity on referencing plugin module

a072d0c

Co-authored-by: Daniël van Noord <[email protected]>

Refer to PyPI and local source explicitly

7eea2f1

This is a much clearer way of explaining what is meant by this recommendation. Co-authored-by: Daniël van Noord <[email protected]>

Merge branch 'main' into issue-7264-clearer-doc-on-plugin-sourcing

11b15de

Pierre-Sassoulas added Documentation 📗 Skip news 🔇 This change does not require a changelog entry labels Aug 25, 2022

Pierre-Sassoulas approved these changes Aug 25, 2022

View reviewed changes

doc/development_guide/how_tos/custom_checkers.rst Show resolved Hide resolved

Pierre-Sassoulas removed the Skip news 🔇 This change does not require a changelog entry label Aug 25, 2022

DanielNoord added the Blocked 🚧 Blocked by a particular issue label Aug 25, 2022

Pierre-Sassoulas mentioned this pull request Aug 26, 2022

Failure with pylint-2.15.0: pylint_plugin_utils.NoSuchChecker: <class 'pylint.checkers.typecheck.TypeChecker'> pylint-dev/pylint-plugin-utils#27

Closed

daogilvie closed this by deleting the head repository Sep 7, 2022

daogilvie reopened this Sep 7, 2022

This was referenced Sep 16, 2022

Add and fix tests for init-hook based plugin load #7475

Merged

Cannot get pylint to register custom checker, is there a missing step in the docs? #7264

Closed

daogilvie requested a review from DanielNoord September 17, 2022 19:14

DanielNoord requested changes Sep 18, 2022

View reviewed changes

DanielNoord removed the Blocked 🚧 Blocked by a particular issue label Sep 18, 2022

DanielNoord added the Skip news 🔇 This change does not require a changelog entry label Sep 18, 2022

daogilvie closed this Sep 20, 2022

daogilvie mentioned this pull request Sep 20, 2022

Improve docs clarity on loading custom checkers - FIXED #7502

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve docs clarity on loading custom checkers #7276

Improve docs clarity on loading custom checkers #7276

daogilvie commented Aug 8, 2022

DanielNoord left a comment

DanielNoord Aug 8, 2022

daogilvie Aug 8, 2022

daogilvie commented Aug 8, 2022

coveralls commented Aug 8, 2022 •

edited

Loading

DanielNoord left a comment

daogilvie commented Aug 8, 2022

daogilvie commented Aug 8, 2022

DanielNoord commented Aug 8, 2022

Pierre-Sassoulas left a comment

Pierre-Sassoulas commented Sep 7, 2022

daogilvie commented Sep 7, 2022

DanielNoord Sep 18, 2022

DanielNoord Sep 18, 2022

daogilvie commented Sep 19, 2022

Pierre-Sassoulas commented Sep 19, 2022

daogilvie commented Sep 20, 2022

daogilvie commented Sep 20, 2022

Pierre-Sassoulas commented Sep 20, 2022

	will also work, but only if:
	will also work, but only if either:

		@@ -0,0 +1,3 @@
		Dev documentation is now clearer about custom checker loading.

Improve docs clarity on loading custom checkers #7276

Improve docs clarity on loading custom checkers #7276

Conversation

daogilvie commented Aug 8, 2022

Type of Changes

Description

Short

Long (in the commit message)

DanielNoord left a comment

Choose a reason for hiding this comment

DanielNoord Aug 8, 2022

Choose a reason for hiding this comment

daogilvie Aug 8, 2022

Choose a reason for hiding this comment

daogilvie commented Aug 8, 2022

coveralls commented Aug 8, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3008066091

💛 - Coveralls

DanielNoord left a comment

Choose a reason for hiding this comment

daogilvie commented Aug 8, 2022

daogilvie commented Aug 8, 2022

DanielNoord commented Aug 8, 2022

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas commented Sep 7, 2022

daogilvie commented Sep 7, 2022

DanielNoord Sep 18, 2022

Choose a reason for hiding this comment

DanielNoord Sep 18, 2022

Choose a reason for hiding this comment

daogilvie commented Sep 19, 2022

Pierre-Sassoulas commented Sep 19, 2022

daogilvie commented Sep 20, 2022

daogilvie commented Sep 20, 2022

Pierre-Sassoulas commented Sep 20, 2022

coveralls commented Aug 8, 2022 •

edited

Loading