sqlalchemy / alembic

A database migrations tool for SQLAlchemy.
MIT License
2.61k stars 234 forks source link

stamp command type annotation prohibits use of `None` revision #1420

Closed ctheune closed 4 months ago

ctheune commented 5 months ago

Describe the bug

When using the stamp() command with None as the revision, this causes a mypy error:

    alembic.command.stamp(alembic_cfg, None, purge=True)

Leads to

    40: error: Argument 2 to "stamp" has incompatible type "None"; expected "str | list[str] | tuple[str, ...]"  [arg-type]

This use case is generated by the (pyramid cookie cutter template for sqlalchemy)[https://github.com/Pylons/pyramid-cookiecutter-starter/blob/latest/%7B%7Bcookiecutter.repo_name%7D%7D/tests/sqlalchemy_conftest.py#L35]

This appears to do what we need but (basically cleaning up the revision table) without resorting to lower level knowledge about alembic's implementation details.

Expected behavior

I'm expecting mypy not to complain. ;)

To Reproduce Please try to provide a Minimal, Complete, and Verifiable example, with the migration script and/or the SQLAlchemy tables or models involved. See also Reporting Bugs on the website.

See above.

Error

See above.

Versions.

Additional context

This might not be considered a true bug, maybe just the request to make this currently implicit behaviour more explicit.

Have a nice day!

You too!

CaselIT commented 5 months ago

Hi,

At the moment passing revision as None is not safe to do to stamp. Example setting sql=True will result in a crash telling that None is not iterable.

Apparently it seems to work in this case when sql=False is used. @zzzeek could this usage be endorsed?

CaselIT commented 5 months ago

Re-looking the interface, I think passing an empty sequence would achieve the same thing as passing None does now, so probably the typing are currently fine as they are

zzzeek commented 5 months ago

you can also pass the string "base", I think this is the official way to say "the base / null revision". does passing "base" work?

CaselIT commented 5 months ago

base indeed works. will improve the docs

sqla-tester commented 5 months ago

Federico Caselli has proposed a fix for this issue in the main branch:

Improve commands doc strings https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5156

ctheune commented 5 months ago

Thanks all for the clarification! I'll adapt to this and will hand an update to the pyramid cookie cutter.