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

process_revision_directives documentation disagrees with type annotation #1325

Closed
jwodder opened this issue Oct 15, 2023 · 5 comments
Closed
Labels
bug Something isn't working documentation pep 484 typing related issues

Comments

@jwodder
Copy link
Contributor

jwodder commented Oct 15, 2023

The documentation for EnvironmentContext.configure.process_revision_directives states that the callable's third argument is a list of MigrationScripts, yet the type annotations in the code state that the third argument is a list of MigrateOperations, a superclass of MigrationScript.

This causes problems in particular when trying to add type annotations to the code from the "Don't Generate Empty Migrations with Autogenerate" recipe, as use of the upgrade_ops property assumes that the list elements are MigrationScripts.

To Reproduce

Running mypy 1.6.0 (default config) on the following code:

from __future__ import with_statement
import logging
from alembic import context
from alembic.migration import MigrationContext
from alembic.operations.ops import MigrationScript
import sqlalchemy as sa

config = context.config
logger = logging.getLogger("alembic.env")

engine: sa.Engine
metadata: sa.MetaData

def process_revision_directives(
    context: MigrationContext,  # noqa: U100
    revision: tuple[str, str],  # noqa: U100
    directives: list[MigrationScript],
) -> None:
    if getattr(config.cmd_opts, "autogenerate", False):
        script = directives[0]
        if script.upgrade_ops.is_empty():
            directives[:] = []
            logger.info("No changes in schema detected.")

with engine.connect() as connection:
    context.configure(
        connection=connection,
        target_metadata=metadata,
        process_revision_directives=process_revision_directives,
    )
    with context.begin_transaction():
        context.run_migrations()

fails with the error:

sample02.py:29: error: Argument "process_revision_directives" to "configure" has incompatible type "Callable[[MigrationContext, tuple[str, str], list[MigrationScript]], None]"; expected "Callable[[MigrationContext, tuple[str, str], list[MigrateOperation]], None] | None"  [arg-type]

If MigrationScript is changed to MigrateOperation, the error changes to:

sample01.py:21: error: "MigrateOperation" has no attribute "upgrade_ops"  [attr-defined]

Versions.

  • OS: macOS 14.0
  • Python: 3.11.6
  • Alembic: 1.12.0
  • SQLAlchemy: 2.0.22
@jwodder jwodder added the requires triage New issue that requires categorization label Oct 15, 2023
@zzzeek
Copy link
Member

zzzeek commented Oct 17, 2023

hi -

for simple documentation issues we can accept PRs to correct problems. thanks!

@zzzeek zzzeek added documentation PRs (with tests!) welcome and removed requires triage New issue that requires categorization labels Oct 17, 2023
@jwodder
Copy link
Contributor Author

jwodder commented Oct 17, 2023

@zzzeek So is the mistake simply that the third argument should be documented as a list of MigrateOperationss? Then how is it safe for the "Don’t Generate Empty Migrations with Autogenerate" recipe to access an attribute that only exists on a subclass?

@zzzeek
Copy link
Member

zzzeek commented Oct 17, 2023

OK. the attribute is in practice a MigrationScript, I will attempt to change the typing to show MigrationScript all the way through and rewrite the doc example to be typed.

@sqla-tester
Copy link
Collaborator

Mike Bayer has proposed a fix for this issue in the main branch:

match process_revision_directives typing to API https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/4927

@zzzeek zzzeek added bug Something isn't working pep 484 typing related issues and removed PRs (with tests!) welcome labels Oct 17, 2023
@zzzeek
Copy link
Member

zzzeek commented Oct 17, 2023

for the next release 1.12.1

sqlalchemy-bot pushed a commit that referenced this issue Oct 24, 2023
Fixed regression caused by 🎫`879` released in 1.7.0 where the
".info" dictionary of ``Table`` would not render in autogenerate create
table statements.  This can be useful for custom create table DDL rendering
schemes so it is restored.

Additionally upon seeing that Rewriter is failing typing that was
just imporved in the previous commit for #1325 /
Ibfb7a57a081818c290cf0964d12a72b85c2c1983, further correct the typing
of the "revision" argument for process_revision_directives which was
still inconsistent.

Change-Id: Ifa4c7bd1b730d51629f42bc159b994f42d157c04
Fixes: #1329
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation pep 484 typing related issues
Projects
None yet
Development

No branches or pull requests

3 participants