Skip to content
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

User-plugin system #3512

Closed
wants to merge 6 commits into from
Closed

User-plugin system #3512

wants to merge 6 commits into from

Conversation

chadrik
Copy link
Contributor

@chadrik chadrik commented Jun 8, 2017

Here's a design sketch for #1240.

Some thoughts, copied from there:

Plugin discovery options

  • A. by file path. e.g. /path/to/myplugin.py. could also extend this with a MYPY_PLUGIN_PATH
    • pro: easier to write test cases (I discovered that placing a file on the PYTHONPATH within the tests was difficult, likely by design)
    • con: can't use pip to install plugins
  • B. by dotted path: e.g. package.module
    • pro: easy for users to create pip-installable plugins
    • con: adding plugin modules and their requirements to the PYTHONPATH could interfere with type checking?
  • C: setuptools entry points. e.g.:
    setup(
        entry_points={
            'mypy.userplugin': ['my_plugin = my_module.plugin:register_plugin']
        }
    )

Sub-Options

  • shall we always look for an object within the module with a designated named, e.g. Plugin, or make this configurable as well? e.g. package.module.MyPlugin or /path/to/myplugin.py:MyPlugin (Note: I've already written some functionality for the latter in my hooks branch)
  • do user plugins need to inherit from mypy.plugin.Plugin?

Plugin chainability options

  • A. aggregate all user plugins into a single uber-plugin instance.
    • each method on this aggregate plugin would cycle through its children in order until one returns a non-None result. we could then cache the mapping from feature (e.g. 'typing.Mapping.get') to user-plugin instance to speed up future lookups.
    • this is compatible with the current design which passes a single Plugin instance around.
  • B. register a plugin per feature. this allows you to replace the search with a fast dictionary lookup, as well as detect up-front when two plugins contend for the same feature.

Right now I'm using discovery option B and chainability option A, but this is just a starting point for debate.

@@ -113,6 +117,9 @@ def __init__(self) -> None:
self.debug_cache = False
self.quick_and_dirty = False

# plugins
self.plugins = [] # type: List[PluginRegistry]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is producing an error when running the tests that I don't understand:

mypy/options.py:121: error: Invalid type "mypy.plugin.PluginRegistry"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that this is because of a reference cycle. There is a known bug that mypy does not treat named tuples in cycles correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the heads up. I suspected a cycle too, but knew that they seemed to be working in general. I'll see if I can resolve the cycle elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for information here is the issue about cycles #3054

@chadrik
Copy link
Contributor Author

chadrik commented Jun 8, 2017

Odd that only 3.5.1 seems to have a problem importing typing.Type on travis.

Two advantages:
- simplifies mypy.build
- related to instantiating user plugins will occur sooner (in main instead of build)
@JelleZijlstra
Copy link
Member

The typing module is provisional and some features were added halfway through the 3.5 series. I believe we test 3.5.1 because that's what Dropbox uses internally.

@gvanrossum
Copy link
Member

The typing module is provisional and some features were added halfway through the 3.5 series. I believe we test 3.5.1 because that's what Dropbox uses internally.

Yes that's exactly right.

This provides a bit more flexibility and also let's us get around the fact that typing.Type does not exist in python 3.5.1.
@chadrik
Copy link
Contributor Author

chadrik commented Jun 9, 2017

Ok, worked around it.

@gvanrossum gvanrossum added the topic-plugins The plugin API and ideas for new plugins label Jun 13, 2017
@JukkaL
Copy link
Collaborator

JukkaL commented Jun 14, 2017

I assume that this can be closed now that #3517 was merged? If there are features not included in #3517 they can potentially be re-engineered as separate PRs on top of #3517.

@JukkaL
Copy link
Collaborator

JukkaL commented Aug 15, 2017

@chadrik Closing this now. If you have further improvements to the plugin system, please send them as new PRs.

@JukkaL JukkaL closed this Aug 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-plugins The plugin API and ideas for new plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants