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

Plugin initialization hook #5911

Open
mkurnikov opened this issue Nov 18, 2018 · 4 comments
Open

Plugin initialization hook #5911

mkurnikov opened this issue Nov 18, 2018 · 4 comments
Labels
feature priority-2-low topic-plugins The plugin API and ideas for new plugins

Comments

@mkurnikov
Copy link
Contributor

mkurnikov commented Nov 18, 2018

I have already asked about augmenting AST with new nodes in the initialization hook for plugins in
#5910

I was able to figure it out with get_back_class_hook and dummy base class like here
https://github.com/mkurnikov/django-stubs/blob/master/django-stubs/conf/__init__.pyi#L7

There's another use case.
In Django there's an INSTALLED_APPS setting:

INSTALLED_APPS = ('app1', 'app2')

which specifies apps to use to retrieve model definitions for database schema (using models.py files).

Those could be used in the model definition code like this

from django.db import models

class User(models.Model):
    member = models.ForeignKey(to='app1.Model1')

In plugin, I can parse to= value, figure out the right type of a model, and set as return type of __get__ to have a proper value for member attribute.
For that I need to use lookup_fully_qualified
https://github.com/python/mypy/blob/master/mypy/semanal.py#L3453
and this requires module to be present in the self.modules list.

I need to create new MypyFile instances to populate that attr myself, but parse_file() method belongs to BuildManager class
https://github.com/python/mypy/blob/master/mypy/build.py#L648
and I wasn't able to find how to access it from the ClassDefContext.

@ilevkivskyi
Copy link
Member

It looks like you have a wrong impression about how mypy works. One does not simply parse a random file :-) We have considered some possible solutions for foreign keys. There are essentially two options:

  • Support settings for plugins where a user can list all modules that contain models.
  • Require an explicit if TYPE_CHECKING: ... import for a model

Taking into account that both solutions will require some action from a user, and the second option already works, this is what we are going to do in short term. Later we can consider another options when we will have more experience with real code.

A general comment/suggestion: don't try to implement everything at once, I would recommend releasing some 0.1-alpha version of the stubs/plugins package with some basic functionality to gather some feedback from users. Then we can adjust plugin API accordingly.

@ilevkivskyi ilevkivskyi added feature priority-2-low topic-plugins The plugin API and ideas for new plugins labels Nov 18, 2018
@mkurnikov
Copy link
Contributor Author

mkurnikov commented Nov 18, 2018

Yes, I've realized it already. The problem is worse, those app1's model definition never appear in the dependencies graph, and never parsed at all because of that. To work with INSTALLED_APPS properly, there should be

  1. Hook like gather_module_dependencies for every module parsed in the mypy.build.load_graph
  2. Some way to append to the seed files from the plugin (could be done via options editing in the plugin's __init__ method already, I think)

Then, settings file could be parsed, and installed apps retrieved from a hook.

It's all sounds like a lot of new features to allow only one Django plugin to work, though. Guess I'll just patch mypy submodule in the repo for now.

Regarding the suggestion: I'm trying to get to a false-positive free state on the internal project in my company, 95% of the errors are those related to the to= as a string in the ForeignKey/OneToOneField. Then I'll release an alpha version, otherwise it's not very useful.

@mkurnikov
Copy link
Contributor Author

Arghh, second option requires #5909 to be implemented and modules to parse be available in the Options.

@ilevkivskyi
Copy link
Member

I'm trying to get to a false-positive free state on the internal project in my company, 95% of the errors are those related to the to= as a string in the ForeignKey/OneToOneField.

You can also just put some Anys instead of some unknown things at this early stage. We use this approach with SQLAlchemy, there are very few false positives, because many things are either permissive or just Any, and people are still happy because this is better than nothing.

(Also patching your local submodule is great for experiments but will have not effect for users who use "stock" mypy.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature priority-2-low topic-plugins The plugin API and ideas for new plugins
Projects
None yet
Development

No branches or pull requests

2 participants