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

consider rationale for ModifyTableOps emitting "pass" for no operations #550

Closed
RazerM opened this issue Apr 1, 2019 · 3 comments

Comments

@RazerM
Copy link
Contributor

commented Apr 1, 2019

I was using alembic's rewrite feature to ignore comment changes (temporarily, not important for the issue):

@writer.rewrites(ops.AlterColumnOp)
def rewrite_alter_column(context, revision, op):
    op.modify_comment = False
    if not op.has_changes():
        return []
    return op

the resulting migration file has many pass lines:

def upgrade():
    pass
    pass
    pass
    ...

It would be nice if alembic wouldn't add unnecessary pass lines.

I found the cause:

return ["pass"]


I found a workaround by rewriting ModifyTableOps after the first rewriter has finished:

writer1 = Rewriter()
writer2 = Rewriter()

@writer1.rewrites(ops.AlterColumnOp)
def rewrite_alter_column(context, revision, op):
    op.modify_comment = False
    if not op.has_changes():
        return []
    return op

@writer2.rewrites(ops.ModifyTableOps)
def rewrite_modify_table_ops(context, revision, op):
    if op.is_empty():
        return []
    return op

writer = writer1.chain(writer2)
@zzzeek zzzeek changed the title rewriting results in many `pass` lines consider rationale for ModifyTableOps emitting "pass" for no operations Apr 1, 2019
@zzzeek

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

need to determine rationale why "pass" is present, no tests cover the code currently:

diff --git a/alembic/autogenerate/render.py b/alembic/autogenerate/render.py
index 21bbb50..698ef63 100644
--- a/alembic/autogenerate/render.py
+++ b/alembic/autogenerate/render.py
@@ -114,7 +114,7 @@ def _render_modify_table(autogen_context, op):
 
         return lines
     else:
-        return ["pass"]
+        return [""]
 
 
 @renderers.dispatch_for(ops.CreateTableCommentOp)

need to determine if there is any impact to removing this additional "pass" as well as to provide test coverage. Contirbutors can start here by writing tests for this code and submitting a PR that includes the change + tests

@zzzeek zzzeek added this to the fasttrack milestone Apr 1, 2019
@sqla-tester

This comment has been minimized.

Copy link
Collaborator

commented Sep 17, 2019

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

Render a single "pass" only for UpgradeOps, DowngradeOps https://gerrit.sqlalchemy.org/1465

@zzzeek

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

we do need to render a "pass" if you were to rewrite all the ops to have no elements, it's just the "pass" needs to be only once at the top level if no lines rendered

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.