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

DropIndexOp does not render keyword arguments such as postgresql_concurrently #849

Closed
jetzhou opened this issue May 25, 2021 · 7 comments
Closed
Labels
autogenerate - rendering bug Something isn't working

Comments

@jetzhou
Copy link
Contributor

jetzhou commented May 25, 2021

Describe the bug
1011366 added kwargs support to drop index operation in SQL generation. However, in autogenerate render step, an operation such as this one:

alembic.operations.ops.DropIndexOp(
    'example_index',
    postgresql_concurrently=True,
)

will be rendered to

op.drop_index(op.f('example_index'))

which ignores the postgresql_concurrently kwarg.

Expected behavior

alembic.operations.ops.DropIndexOp(
    'example_index',
    postgresql_concurrently=True,
)

should render in autogenerate to

op.drop_index(op.f('example_index'), postgresql_concurrently=True)

To Reproduce
Test code that does not pass

    def test_render_drop_index_custom_kwarg(self):
        t = Table(
            "test",
            MetaData(),
            Column("id", Integer, primary_key=True),
            Column("active", Boolean()),
            Column("code", String(255)),
        )
        idx = Index(None, t.c.active, t.c.code, somedialect_foobar="option")
        op_obj = ops.DropIndexOp.from_index(idx)
        eq_ignore_whitespace(
            autogenerate.render_op_text(self.autogen_context, op_obj),
            "op.drop_index(op.f('ix_test_active'), table_name='test', "
            "somedialect_foobar='option')",
        )

Versions.

  • OS: Ubuntu 21.04
  • Python: 3.9.5
  • Alembic: 1.6.4
  • SQLAlchemy: 1.3.19
  • Database: PostgreSQL
  • DBAPI: psycopg

Additional context
Proposed fix: jetzhou@6392a28

Have a nice day!

@jetzhou jetzhou added the requires triage New issue that requires categorization label May 25, 2021
@zzzeek zzzeek added autogenerate - rendering bug Something isn't working and removed requires triage New issue that requires categorization labels May 27, 2021
@zzzeek
Copy link
Member

zzzeek commented May 27, 2021

will have to look and see if we have existing precedent for this, which I think we should, like for indexes etc.

@zzzeek
Copy link
Member

zzzeek commented May 27, 2021

looks like you have a fix ready! that's a PR, please send. I assume most of the other constructs are OK in this regard

@jetzhou
Copy link
Contributor Author

jetzhou commented May 27, 2021

Thanks @zzzeek for the triage! Created PR for this.

@zzzeek
Copy link
Member

zzzeek commented May 27, 2021

thanks for following all the guidelines on this! most people dont :) but it makes it easy

@sqla-tester
Copy link
Collaborator

Jet Zhou has proposed a fix for this issue in the master branch:

Add kwargs support to DropIndexOp autogenerate render https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/2844

@zzzeek
Copy link
Member

zzzeek commented May 27, 2021

thanks! 1.6.5 is released.

@jetzhou
Copy link
Contributor Author

jetzhou commented May 27, 2021

Woohoo! Thanks for taking a look and the quick + efficient PR prorcess!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autogenerate - rendering bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants