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

Support if_exists and if_not_exists on create/drop table commands #1521

Closed

Conversation

agriffin-grow
Copy link
Contributor

Fixes: #1520

Description

Adds if_exists/if_not_exists support to table commands, mirroring closely the same logic for indexes.

Checklist

This pull request is:

  • A documentation / typographical error fix
    • Good to go, no issue or tests are needed
  • A short code fix
    • please include the issue number, and create an issue if none exists, which
      must include a complete example of the issue. one line code fixes without an
      issue and demonstration will not be accepted.
    • Please include: Fixes: #<issue number> in the commit message
    • please include tests. one line code fixes without tests will not be accepted.
  • A new feature implementation
    • please include the issue number, and create an issue if none exists, which must
      include a complete example of how the feature would look.
    • Please include: Fixes: #<issue number> in the commit message
    • please include tests.

Have a nice day!

:param if_not_exists: If True, adds IF NOT EXISTS operator when
creating the table.

.. versionadded:: 1.13.4
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really sure if this is the correct next version. Happy to change it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be 1.13.3 since it's yet to be released

Copy link
Member

@CaselIT CaselIT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work. It's just missing a changelog: see example in https://github.com/sqlalchemy/alembic/tree/main/docs/build/unreleased

Feel free to add something like "pull request courtesy of "

sorry for the delay in the review

:param if_not_exists: If True, adds IF NOT EXISTS operator when
creating the table.

.. versionadded:: 1.13.4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be 1.13.3 since it's yet to be released

@lachaib
Copy link
Contributor

lachaib commented Aug 28, 2024

Hello,

My 2 cents on this PR, which I'm looking forward, is that it's missing the same thing I'm trying to fix with #1446 : we should have a small addition in alembic/autogenerate/render.py to render the option from autogen context.
It'd be great to have it from the start.

Thanks a lot for this pull request

Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 88c7b3f of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

New Gerrit review created for change 88c7b3f: https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5455

Copy link
Member

@zzzeek zzzeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the tools/write_pyi.py script needs to be run here to generate appropriate stub files

@agriffin-grow
Copy link
Contributor Author

@CaselIT @zzzeek updated based on your comments.

I also bumped against #1524 when using tools/write_pyi.py and put in a PR for that.

Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 1063a5a of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset 1063a5a added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5455

@zzzeek
Copy link
Member

zzzeek commented Sep 1, 2024

hi -

still some failures here you may need to add skip markers to these tests for SQLAlchemy < 1.4

@agriffin-grow
Copy link
Contributor Author

hi -

still some failures here you may need to add skip markers to these tests for SQLAlchemy < 1.4

TY, I missed that tests still run for sqlalchemy 1.3 - should be updated now

Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 747ae3f of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset 747ae3f added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5455

@sqla-tester
Copy link
Collaborator

Michael Bayer (zzzeek) wrote:

recheck

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5455

Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Michael Bayer (zzzeek) wrote:

code review left on gerrit

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5455

tox.ini Outdated Show resolved Hide resolved
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision ab4e7d4 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset ab4e7d4 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5455

Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 469be01 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset 469be01 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5455

Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Michael Bayer (zzzeek) wrote:

code review left on gerrit

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5455

  • tox.ini (line 8): Done

@sqla-tester
Copy link
Collaborator

Federico Caselli (CaselIT) wrote:

Great work!

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5455

@agriffin-grow
Copy link
Contributor Author

FYI I think @zzzeek might need to approve as well on account of the requested changes... but I also have no idea how Github and Gerrit interact with each other :)

@sqla-tester
Copy link
Collaborator

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5455 has been merged. Congratulations! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support if_exists / if_not_exists on drop_table and create_table operations
5 participants