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

Allow use of entry-points like strings in mypy.ini to register plugins #5358

Merged
merged 5 commits into from Aug 3, 2018

Conversation

Projects
None yet
6 participants
@chadrik
Copy link
Contributor

chadrik commented Jul 15, 2018

This addresses issue #3916. Unlike my last attempt this does not use pkg_resources.

It is currently possible for third-parties to author plugins and load them via mypy.ini, using a path to a plugin file. e.g.

[mypy]
plugins = path/plugin1.py, path/plugin2.py

This PR extends the syntax to allow plugins to be specified using entry-points-like strings:

[mypy]
plugins = path/plugin1.py, plugin2:register

Reservations have been expressed about the still-experimental nature of the plugin API. In rebuttal, I would say:

  • It's already possible to load third-party plugins, it's just inconvenient
  • Allowing developers to synchronize release of their plugins with their libraries will ensure compatibility and help reduce spurious bug reports.

Perhaps a compromise is to simply not advertise this feature extensively yet. It will enable projects like attrs, which are at the bleeding edge of static type checking and well aware of the unsupported risks involved, to push the tooling forward, while shouldering the responsibility of making adjustments to our plugin if the API changes.

@gvanrossum

This comment has been minimized.

Copy link
Member

gvanrossum commented Jul 15, 2018

I think it's fine to add this. Yes, the plugin API will still change rapidly (basically each time someone writes a new plugin they have to spend some time changing the existing API), but since we already have plugin configuration in mypy.ini I don't see how this makes it worse.

(Haven't had time to review the code yet, sorry.)

@ethanhs

This comment has been minimized.

Copy link
Collaborator

ethanhs commented Jul 15, 2018

I think it's fine to add this. Yes, the plugin API will still change rapidly (basically each time someone writes a new plugin they have to spend some time changing the existing API), but since we already have plugin configuration in mypy.ini I don't see how this makes it worse.

I agree. I think having a way to point mypy to a plugin has gotten significantly more important as several projects (such as attrs and numpy-stubs) will likely need this. With PEP 561 support, I only expect the number of projects wanting this to increase.

I will take a look at reviewing shortly.

@chadrik

This comment has been minimized.

Copy link
Contributor Author

chadrik commented Jul 15, 2018

Great! I'll go over this a bit more thoroughly (I haven't actually tested it yet) and get all tests passing.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Jul 15, 2018

I actually wanted to do something similar myself. Using a qualified name of plugin seems much simpler and sustainable than the full path. Didn't look at the code yet, but a quick question now: why such special syntax (I mean the colon)? I would expect just a qualified name, i.e. plugins = path/to/plugin1.py, package.plugin2 (IOW the same thing one would write in an import statement).

@ethanhs

This comment has been minimized.

Copy link
Collaborator

ethanhs commented Jul 15, 2018

why such special syntax (I mean the colon)?

This is used in entry points for packaging, so I think the idea is to mimic that.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Jul 15, 2018

This is used in entry points for packaging, so I think the idea is to mimic that.

What I would care is whether this is a common syntax for plugins in other Python projects. If not, I would stick to something simpler. Also there is a certain asymmetry with the current syntax, for the full path we always look for a function called plugin. I would expect the same for the new syntax, I would just expect a module instead of path.

@chadrik

This comment has been minimized.

Copy link
Contributor Author

chadrik commented Jul 15, 2018

Btw, I support omitting the function name, as in the third example:

[mypy]
plugins = path/plugin1.py, plugin2:register, plugin3

In the last case, the registration function defaults to plugin, as it does for the first case.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Jul 15, 2018

Btw, I support omitting the function name

OK, then I am fine with this. As I understand the module name can be qualified, like package.plugin.

Btw, maybe we can also add the possibility to override the function name with a colon syntax for the case of full path (for consistency)?

@chadrik

This comment has been minimized.

Copy link
Contributor Author

chadrik commented Jul 15, 2018

Btw, maybe we can also add the possibility to override the function name with a colon syntax for the case of full path (for consistency)

I just realized that we cannot currently tell whether a plugin registered as foo.py is a file or a qualified module name (py.py is a valid python file name). Here are a few options:

  1. make the entry-point required for modules: foo.py is a file and foo.py:register is a module entry-point.
  2. require that paths include at least one slash: ./foo.py is a file and foo.py is a module.
  3. remove support for file paths (I seriously doubt they see much real-world use)
  4. choose a new key for module plugins, other than plugins
  5. ignore the problem as a niche case

I will defer to you all on this.

@refi64

This comment has been minimized.

Copy link
Contributor

refi64 commented Jul 15, 2018

Personal opinion: choice 2 would have the advantage of being explicit as to the file's location. It's immediately obvious that this is looking in the current directory, not a special lookup path of some sort or similar.

@chadrik

This comment has been minimized.

Copy link
Contributor Author

chadrik commented Jul 15, 2018

choice 2 would have the advantage of being explicit as to the file's location.

It's not backward incompatible, though. If we're willing to make a breaking change, then why not option 3? In that case, if someone is currently using foo.py as a file path, they can change it to foo, and it should work when run from the same directory as the config file.

@gvanrossum

This comment has been minimized.

Copy link
Member

gvanrossum commented Jul 15, 2018

@chadrik

This comment has been minimized.

Copy link
Contributor Author

chadrik commented Jul 15, 2018

So option 5 then ;)

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Jul 15, 2018

I am with Guido here.

@chadrik

This comment has been minimized.

Copy link
Contributor Author

chadrik commented Jul 15, 2018

I added some tests.

For certain key cases related to the loading mechanism, I test both the file and module style loading, but what do you think about converting the remainder to use the module-style by default?

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Jul 21, 2018

@chadrik Thanks for adding tests! I like the PR in general, but it looks like I will not have time to review this in detail before my vacation. I hope someone else can review and merge this. I have only one comment, I would prefer to see "sandboxed" tests for installed plugins, e.g. like @ethanhs did this for PEP 561 packages testing instead of modifying sys.path.

@ethanhs

This comment has been minimized.

Copy link
Collaborator

ethanhs commented Jul 22, 2018

it looks like I will not have time to review this in detail before my vacation. I hope someone else can review and merge this.

Enjoy your well earned vacation! I am happy to take this over, unless anyone else would prefer to take it (go ahead and unassign me if so).

@ethanhs ethanhs self-assigned this Jul 22, 2018

@gvanrossum

This comment has been minimized.

Copy link
Member

gvanrossum commented Jul 22, 2018

Thanks -- please go ahead, Ethan!

@chadrik

This comment has been minimized.

Copy link
Contributor Author

chadrik commented Jul 24, 2018

I have only one comment, I would prefer to see "sandboxed" tests for installed plugins

The existing plugin tests use testcheck.py which runs in the current process. Pushing this into a subprocess will necessitate a specialized runner for plugins, which substantially increases the scope
of this PR. It feels like the right thing to do here is to make a separate issue for plugin test sandboxing.

@ethanhs

This comment has been minimized.

Copy link
Collaborator

ethanhs commented Jul 28, 2018

I think ideally the plugin tests should have their own suite which is run out of process. The check suite is supposed to test the core type checking features of mypy, a plugin seems like a different unit to test. That being said, if you feel like it would be easier, I think there will plenty of time to refactor the testing in a separate PR.

@chadrik

This comment has been minimized.

Copy link
Contributor Author

chadrik commented Jul 29, 2018

@JukkaL

This comment has been minimized.

Copy link
Collaborator

JukkaL commented Jul 30, 2018

I usually prefer having non-trivial refactorings as separate PRs. PRs with both refactorings and new features are slower to review. They are also more likely to get merge conflicts, resulting in further delays. Sometimes it's best to do the refactoring as the first PR, but this is up to the author.

@chadrik

This comment has been minimized.

Copy link
Contributor Author

chadrik commented Jul 31, 2018

@ethanhs

This comment has been minimized.

Copy link
Collaborator

ethanhs commented Jul 31, 2018

What’s left to do on this one?

I will do a final review later today and hopefully merge.

@ethanhs
Copy link
Collaborator

ethanhs left a comment

Just two minor things. Thank you for working on this!

@@ -580,33 +581,43 @@ def plugin_error(message: str) -> None:
errors.set_file(options.config_file, None)
for plugin_path in options.plugins:
# Plugin paths are relative to the config file location.

This comment has been minimized.

@ethanhs

ethanhs Aug 1, 2018

Collaborator

This comment seems out of date.

This comment has been minimized.

@chadrik

chadrik Aug 1, 2018

Author Contributor

true. will get this addressed once I get your answer on the second issue.

fnam = os.path.basename(plugin_path)
module_name = fnam[:-3]
sys.path.insert(0, plugin_dir)
elif re.search(r'\/', plugin_path):

This comment has been minimized.

@ethanhs

ethanhs Aug 1, 2018

Collaborator

This won't work correctly on Windows if someone gives a path with e.g C:\\Foo\\bar. I'd recommend os.sep in plugin_path instead.

This comment has been minimized.

@chadrik

chadrik Aug 1, 2018

Author Contributor

Thanks for catching that. What I intended to do was this:

re.search(r'[\\/]', plugin_path) 

I considered os.sep, but decided against it because it's bound to the current OS. I think it's more correct to detect anything that looks like a file path, but I'll let you make the call.

This comment has been minimized.

@ethanhs

ethanhs Aug 1, 2018

Collaborator

That is a good point. I think your proposed solution is good.

@chadrik

This comment has been minimized.

Copy link
Contributor Author

chadrik commented Aug 3, 2018

Notes addressed

@ethanhs

ethanhs approved these changes Aug 3, 2018

@ethanhs ethanhs merged commit f8d5abb into python:master Aug 3, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ethanhs

This comment has been minimized.

Copy link
Collaborator

ethanhs commented Aug 3, 2018

@chadrik thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.