Skip to content

Add plugin hook for dynamic class definition #5875

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

Merged
merged 3 commits into from
Nov 9, 2018

Conversation

ilevkivskyi
Copy link
Member

Fixes #5508

The new hook allows this:

from some_lib import dynamic_base

Base = dynamic_base()

class C(Base):  # No error, the plugin acts and replaces 'Base' with a TypeInfo
    ...

This plugin hook is useful for SQLAlchemy ORM, for user-defined constructs similar to namedtuple(), and maybe we can even re-implement namedtuple() as a plugin at some point.

Here are some comments:

  • I don't add basic_new_typeinfo() to semantic analyzer API, because it is hard to find a signature that would satisfy all possible use cases. Instead I add just add_symbol_table_node() and qualified_name(), so that a user can construct TypeInfo manually and add it to the symbol table. We can reconsider this when we will have more experience with this hook, and add a dedicated utility method.
  • Semantic analyzer API in semanal_shared.py has add_symbol_table_node(), but I think it is outdated, and should be replaced with add_symbol() which is IMO better (this is unrelated to this PR, but if you agree I will open an issue).
  • If you think the added test case is overcomplicated, this is on purpose. The added test case is a PoC implementation of how this would work in SQLAlchemy, I wanted to be sure that new hook will work well also in combination with the other base class related hook (and I would say this may be a typical combination).

@ilevkivskyi ilevkivskyi requested a review from JukkaL November 8, 2018 17:43
Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Looks good, just a few ideas about potential new tests.

# flags: --config-file tmp/mypy.ini
from mod import declarative_base, Column, Instr

Base = declarative_base()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also test a few different assignment statements and ensure that the plugin doesn't get run them. Maybe things like A = B = ... and C = <other_function>(). If the plugin doesn't get run, the lvalue shouldn't be a valid base class.

[file mypy.ini]
[[mypy]
plugins=<ROOT>/test-data/unit/plugins/dyn_class.py
[builtins fixtures/classmethod.pyi]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to add an AST diff test case for the base class? E.g. if there is no change, the base class shouldn't be triggered.

@ilevkivskyi
Copy link
Member Author

@JukkaL Thanks! I added the tests you suggested.

@JukkaL JukkaL merged commit e5e4532 into python:master Nov 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants