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

Add plugin hook for dynamic class definition #5875

Merged
merged 3 commits into from Nov 9, 2018

Conversation

Projects
None yet
2 participants
@ilevkivskyi
Copy link
Collaborator

ilevkivskyi commented Nov 8, 2018

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 Nov 8, 2018

Ivan Levkivskyi
@JukkaL
Copy link
Collaborator

JukkaL left a comment

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()

This comment has been minimized.

@JukkaL

JukkaL Nov 9, 2018

Collaborator

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]

This comment has been minimized.

@JukkaL

JukkaL Nov 9, 2018

Collaborator

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.

Ivan Levkivskyi
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Nov 9, 2018

@JukkaL Thanks! I added the tests you suggested.

@JukkaL

JukkaL approved these changes Nov 9, 2018

@JukkaL JukkaL merged commit e5e4532 into python:master Nov 9, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment