8000 User-plugin system by chadrik · Pull Request #3512 · python/mypy · GitHub
[go: up one dir, main page]
Skip to content

User-plugin system #3512

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.

Sign up for GitHub

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

Jump to bottom
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.

chadrik
chadrik commented Jun 8, 2017
View reviewed changes
mypy/options.py
@@ -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 •
edited
Loading

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

chadrik added 3 commits June 8, 2017 18:00
@chadrik
Move the creation of PluginManager earlier.
8ec21e5 
Two advantages:
- simplifies mypy.build
- related to instantiating user plugins will occur sooner (in main instead of build)
@chadrik
fix name
67262c4
@chadrik
it's called plugins now not hooks
5e784bc
@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.

@chadrik
Expect plugins to provide a register_plugin function.
8aaebd3 
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.

@JukkaL JukkaL mentioned this pull request Jun 9, 2017
Merged
@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
Reviewers

@ilevkivskyi ilevkivskyi ilevkivskyi left review comments

Assignees
No one assigned
Labels
topic-plugins The plugin API and ideas for new plugins
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chadrik @JelleZijlstra @gvanrossum @JukkaL @ilevkivskyi
0