New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic support for user-defined mypy plugins #3517

Merged
merged 6 commits into from Jun 13, 2017

Conversation

Projects
None yet
5 participants
@JukkaL
Collaborator

JukkaL commented Jun 9, 2017

Configure them through plugins=path/plugin.py, ... in the ini file.
The paths are relative to the configuration file.

This is an almost minimal implementation and some features
are missing:

  • Plugins installed through pip aren't properly supported.
  • Plugins within packages aren't properly supported.
  • Incremental mode doesn't invalidate cache files when
    plugins change.

@chadrik We've been working on the same issue. This seems fairly
similar to your #3512. Not sure what's the best way forward.

Here are some thoughts about the design of this PR:

  • To configure a plugin, a user only needs to remember the name
    (or currently path) of the plugin. Every plugin has a single,
    fixed entry point plugin(mypy_version). The idea is to make
    configuring plugins as easy as possible, as this is the main
    explicit user interface to the plugin system.
  • Since the entry point takes mypy version as an argument, it
    should be possible to write plugins that are compatible across
    multiple mypy versions even if the internal API changes. This
    won't be particularly convenient, though.
  • If there is an exception raised during plugin initialization, we just
    display a normal Python stack trace (along with a note about what
    was going on). Plugin errors aren't really mypy internal errors but
    programmer errors (in this case of the plugin developer) so
    I want to make them very visible.

JukkaL added some commits Jun 8, 2017

Add basic support for custom mypy plugins
Configure them through "plugins=path/plugin.py, ..." in the ini file.
The paths are relative to the configuration file.

This is an almost minimal implementation and some features
are missing:

* Plugins installed through pip aren't properly supported.
* Plugins within packages aren't properly supported.
* Incremental mode doesn't invalidate cache files when
  plugins change.
Attempt to fix path normalization in test cases in Windows
Previously we sometimes normalized to Windows paths and sometimes
to Linux paths. Now switching to always use Linux paths.
def get_method_hook(self, fullname: str) -> Optional[MethodHook]:
return self._find_hook(lambda plugin: plugin.get_method_hook(fullname))
def _find_hook(self, lookup: Callable[[Plugin], T]) -> Optional[T]:

This comment has been minimized.

@chadrik

chadrik Jun 9, 2017

Contributor

The result of this should probably be cached based on hook-type and fullname.

@chadrik

chadrik Jun 9, 2017

Contributor

The result of this should probably be cached based on hook-type and fullname.

This comment has been minimized.

@JukkaL

JukkaL Jun 13, 2017

Collaborator

Created an issue about caching (#3533). This may need some analysis or experimentation to decide a caching strategy (e.g. unlimited cache size vs bounded cache size; maximum size of the cache) so I feel that it's better to do it separately.

@JukkaL

JukkaL Jun 13, 2017

Collaborator

Created an issue about caching (#3533). This may need some analysis or experimentation to decide a caching strategy (e.g. unlimited cache size vs bounded cache size; maximum size of the cache) so I feel that it's better to do it separately.

Show outdated Hide outdated mypy/test/testgraph.py
@@ -42,6 +43,7 @@ def _make_manager(self) -> BuildManager:
reports=Reports('', {}),
options=Options(),
version_id=__version__,
plugin=Plugin((3, 6)),

This comment has been minimized.

@chadrik

chadrik Jun 9, 2017

Contributor

should this use defaults.PYTHON3_VERSION?

@chadrik

chadrik Jun 9, 2017

Contributor

should this use defaults.PYTHON3_VERSION?

This comment has been minimized.

@JukkaL

JukkaL Jun 13, 2017

Collaborator

Updated

@JukkaL

JukkaL Jun 13, 2017

Collaborator

Updated

@chadrik

This comment has been minimized.

Show comment
Hide comment
@chadrik

chadrik Jun 9, 2017

Contributor

Looks really similar to what I have except it includes tests, so you win!

There are two other functional differences that I want to discuss:

Support for plugin options

Some plugins may want to expose user-configurable options. For example, with my docstring parser I want users to be able to specify which style of docstrings to expect (the default behavior of automatic discovery is a bit slower).

Here are three proposals for how to link plugin registration with per-plugin configuration options:


Option A

The correlation here is bit fragile and the per-plugin section headers may be difficult to grok for longer (i.e. absolute) paths:

[mypy]
fast_parser = true
plugins = /path/to/typeddict.py, /path/to/mypydoc.py

[mypy.plugins-/path/to/mypydoc.py]
docstring_style = 'google'

Option B

The following is visually clean, but can't as easily piggy-back on the current options-parsing code. (Note: I believe that mypy-plugins with a dash would conflict with mypy's per-module configuration):

[mypy]
fast_parser = true

[mypy.plugins]
typeddict = /path/to/typeddict.py
mypydoc = /path/to/mypydoc.py

[mypy.plugins-mypydoc]
docstring_style = 'google'

Option C

The following dotted registration style is used heavily by mercurial, and this is what I decided on in my implementation. It piggy-backs existing options parsing code, so it could easily be extended to per-module options in the future, if we found a need for that:

[mypy]
fast_parser = true
plugins.typeddict = /path/to/typeddict.py
plugins.mypydoc = /path/to/mypydoc.py

[mypy.plugins-mypydoc]
docstring_style = 'google'

Support for module plugins

You already mentioned this omission, but it should be fairly trivial to detect a plugin specifier that is path-like (re.search(r'[/\\]|([.]py$)', x)) and use that as a switch to support both behaviors. What do you think?

Contributor

chadrik commented Jun 9, 2017

Looks really similar to what I have except it includes tests, so you win!

There are two other functional differences that I want to discuss:

Support for plugin options

Some plugins may want to expose user-configurable options. For example, with my docstring parser I want users to be able to specify which style of docstrings to expect (the default behavior of automatic discovery is a bit slower).

Here are three proposals for how to link plugin registration with per-plugin configuration options:


Option A

The correlation here is bit fragile and the per-plugin section headers may be difficult to grok for longer (i.e. absolute) paths:

[mypy]
fast_parser = true
plugins = /path/to/typeddict.py, /path/to/mypydoc.py

[mypy.plugins-/path/to/mypydoc.py]
docstring_style = 'google'

Option B

The following is visually clean, but can't as easily piggy-back on the current options-parsing code. (Note: I believe that mypy-plugins with a dash would conflict with mypy's per-module configuration):

[mypy]
fast_parser = true

[mypy.plugins]
typeddict = /path/to/typeddict.py
mypydoc = /path/to/mypydoc.py

[mypy.plugins-mypydoc]
docstring_style = 'google'

Option C

The following dotted registration style is used heavily by mercurial, and this is what I decided on in my implementation. It piggy-backs existing options parsing code, so it could easily be extended to per-module options in the future, if we found a need for that:

[mypy]
fast_parser = true
plugins.typeddict = /path/to/typeddict.py
plugins.mypydoc = /path/to/mypydoc.py

[mypy.plugins-mypydoc]
docstring_style = 'google'

Support for module plugins

You already mentioned this omission, but it should be fairly trivial to detect a plugin specifier that is path-like (re.search(r'[/\\]|([.]py$)', x)) and use that as a switch to support both behaviors. What do you think?

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jun 10, 2017

Member

Regarding plugin options, I recall the options to pytest plugins driving me mad, because they appear to be pytest options but aren't in the pytest docs (and the usage message goes on and on and on...).

A more minimal alternative would be to just have different names for the plugin, one for each variation.

Member

gvanrossum commented Jun 10, 2017

Regarding plugin options, I recall the options to pytest plugins driving me mad, because they appear to be pytest options but aren't in the pytest docs (and the usage message goes on and on and on...).

A more minimal alternative would be to just have different names for the plugin, one for each variation.

@miedzinski

This comment has been minimized.

Show comment
Hide comment
@miedzinski

miedzinski Jun 10, 2017

Contributor

@chadrik
Since mypy can configure itself from setup.cfg and we definitely don't want any naming conflicts, I think all plugin option sections should be somehow marked as mypy-specific (i.e. prefixed with mypy: or mypy-plugin:).

Contributor

miedzinski commented Jun 10, 2017

@chadrik
Since mypy can configure itself from setup.cfg and we definitely don't want any naming conflicts, I think all plugin option sections should be somehow marked as mypy-specific (i.e. prefixed with mypy: or mypy-plugin:).

@chadrik

This comment has been minimized.

Show comment
Hide comment
@chadrik

chadrik Jun 10, 2017

Contributor

@miedzinski good point. I updated my examples above to include proper namespaces.

I tried to come up with some logic for the separators:

  • periods join known mypy sections: e.g. [mypy], [mypy.plugins]
  • dashes join sections with dynamic topics: e.g. [mypy.plugins-custom.plugin], [mypy-custom.module]

I'm completely open to other suggestions. Underscore could work in place of periods, but I found it less visually appealing.

Contributor

chadrik commented Jun 10, 2017

@miedzinski good point. I updated my examples above to include proper namespaces.

I tried to come up with some logic for the separators:

  • periods join known mypy sections: e.g. [mypy], [mypy.plugins]
  • dashes join sections with dynamic topics: e.g. [mypy.plugins-custom.plugin], [mypy-custom.module]

I'm completely open to other suggestions. Underscore could work in place of periods, but I found it less visually appealing.

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Jun 13, 2017

Collaborator

@chadrik I agree that specifying options for plugins would be useful. They can be implemented in a separate PR -- it would also be good to first discuss the best way to provide them (outside this PR). Support for referring to plugins through a module name can also be added with another PR. I usually prefer keeping PRs small to make them easy to review and to keep discussions focused. Feel free to create issues for the new features.

Collaborator

JukkaL commented Jun 13, 2017

@chadrik I agree that specifying options for plugins would be useful. They can be implemented in a separate PR -- it would also be good to first discuss the best way to provide them (outside this PR). Support for referring to plugins through a module name can also be added with another PR. I usually prefer keeping PRs small to make them easy to review and to keep discussions focused. Feel free to create issues for the new features.

@chadrik

This comment has been minimized.

Show comment
Hide comment
@chadrik

chadrik Jun 13, 2017

Contributor

@JukkaL that's fine as long as you don't do a release between this PR and the next, since the signature to the plugin registration function will change (it will grow a new options argument) and the layout of mypy.ini file might change. I'll get a PR for that made soon.

This PR looks good to me.

Contributor

chadrik commented Jun 13, 2017

@JukkaL that's fine as long as you don't do a release between this PR and the next, since the signature to the plugin registration function will change (it will grow a new options argument) and the layout of mypy.ini file might change. I'll get a PR for that made soon.

This PR looks good to me.

@chadrik

This comment has been minimized.

Show comment
Hide comment
@chadrik

chadrik Jun 13, 2017

Contributor

Actually, I forgot I had two review notes above. What do you think about caching the lookup on the ChainedPlugin?

Contributor

chadrik commented Jun 13, 2017

Actually, I forgot I had two review notes above. What do you think about caching the lookup on the ChainedPlugin?

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Jun 13, 2017

Collaborator

I created #3533 about caching.

Collaborator

JukkaL commented Jun 13, 2017

I created #3533 about caching.

@JukkaL JukkaL merged commit fd0a416 into master Jun 13, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
-- Test cases for user-defined plugins
--
-- Note: Plugins used by tests live under test-data/unit/plugins. Defining
-- plugin files in test cases does not work reliably.

This comment has been minimized.

@gvanrossum

gvanrossum Jun 13, 2017

Member

Why not?

@gvanrossum

gvanrossum Jun 13, 2017

Member

Why not?

This comment has been minimized.

@JukkaL

JukkaL Jun 14, 2017

Collaborator

Not sure exactly. I spent some time trying to get it to work and then gave up. A couple of hypotheses:

  • If two test cases use the same plugin name, we need to reliably free the plugin between test cases as otherwise the plugin module will be visible between tests. It may be hard to ensure that the plugin is always deleted at the end of the test case.
  • Python may do some caching / time stamp magic that may cause the import machinery to sometimes not see a recently created module.
@JukkaL

JukkaL Jun 14, 2017

Collaborator

Not sure exactly. I spent some time trying to get it to work and then gave up. A couple of hypotheses:

  • If two test cases use the same plugin name, we need to reliably free the plugin between test cases as otherwise the plugin module will be visible between tests. It may be hard to ensure that the plugin is always deleted at the end of the test case.
  • Python may do some caching / time stamp magic that may cause the import machinery to sometimes not see a recently created module.
[[mypy]
plugins=missing.py
[out]
tmp/missing.py:0: error: Can't find plugin

This comment has been minimized.

@gvanrossum

gvanrossum Jun 13, 2017

Member

This error is confusing. Maybe it would be less confusing if the plugin name as specified in the config file (here "missing.py") was repeated in the error message, and the error mentioned that this was a plugin specified by the config file.

@gvanrossum

gvanrossum Jun 13, 2017

Member

This error is confusing. Maybe it would be less confusing if the plugin name as specified in the config file (here "missing.py") was repeated in the error message, and the error mentioned that this was a plugin specified by the config file.

This comment has been minimized.

@JukkaL

JukkaL Jun 14, 2017

Collaborator

Will update the error message (in a separate PR)

@JukkaL

JukkaL Jun 14, 2017

Collaborator

Will update the error message (in a separate PR)

plugins=badext.pyi
[file badext.pyi]
[out]
tmp/badext.pyi:0: error: Plugin must have .py extension

This comment has been minimized.

@gvanrossum

gvanrossum Jun 13, 2017

Member

This should similarly have a better error message.

@gvanrossum

gvanrossum Jun 13, 2017

Member

This should similarly have a better error message.

This comment has been minimized.

@JukkaL

JukkaL Jun 14, 2017

Collaborator

Ok will update the error message

@JukkaL

JukkaL Jun 14, 2017

Collaborator

Ok will update the error message

[[mypy]
plugins=<ROOT>/test-data/unit/plugins/noentry.py
[out]
<ROOT>/test-data/unit/plugins/noentry.py:0: error: Plugin does not define entry point function "plugin"

This comment has been minimized.

@gvanrossum

gvanrossum Jun 13, 2017

Member

I wonder if line 1 would be less jarring?

@gvanrossum

gvanrossum Jun 13, 2017

Member

I wonder if line 1 would be less jarring?

This comment has been minimized.

@JukkaL

JukkaL Jun 14, 2017

Collaborator

Could be -- I'll give it a try

@JukkaL

JukkaL Jun 14, 2017

Collaborator

Could be -- I'll give it a try

[[mypy]
plugins=<ROOT>/test-data/unit/plugins/badreturn.py
[out]
<ROOT>/test-data/unit/plugins/badreturn.py:0: error: Type object expected as the return value of "plugin" (got None)

This comment has been minimized.

@gvanrossum

gvanrossum Jun 13, 2017

Member

Is there no way to make this list the line number of the plugin() function? Ditto below.

@gvanrossum

gvanrossum Jun 13, 2017

Member

Is there no way to make this list the line number of the plugin() function? Ditto below.

This comment has been minimized.

@JukkaL

JukkaL Jun 14, 2017

Collaborator

It may be possible, but giving the line number of the return statement doesn't seem feasible. I'll look at this.

@JukkaL

JukkaL Jun 14, 2017

Collaborator

It may be possible, but giving the line number of the return statement doesn't seem feasible. I'll look at this.

@gvanrossum gvanrossum deleted the user-plugins branch Jun 13, 2017

JukkaL added a commit that referenced this pull request Jun 14, 2017

@kirbyfan64

This comment has been minimized.

Show comment
Hide comment
@kirbyfan64

kirbyfan64 Jun 15, 2017

Contributor

I know this has already been merged, but would it be possible to also add a command-line parameter for this, to make it easier to be used by IDEs and such?

Contributor

kirbyfan64 commented Jun 15, 2017

I know this has already been merged, but would it be possible to also add a command-line parameter for this, to make it easier to be used by IDEs and such?

JukkaL added a commit that referenced this pull request Jun 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment