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

[view] Support CREATE OR REPLACE #710

Merged
merged 1 commit into from
Jun 29, 2023
Merged

Conversation

mgorven
Copy link
Contributor

@mgorven mgorven commented Jun 9, 2023

No description provided.

Copy link
Collaborator

@kurtmckee kurtmckee left a comment

Choose a reason for hiding this comment

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

@mgorven Thanks for submitting this!

Would you add some tests that exercise this feature? It looks like these tests will be worthwhile:

  • ValueError is raised when materialized and replace are both true
  • A view is created when replace is true and it doesn't exist
  • A view is replaced when replace is true and it already exists

@mgorven
Copy link
Contributor Author

mgorven commented Jun 9, 2023

Sure. Are there any docs on how to run tests locally? It seems like they require a real database.

@kurtmckee
Copy link
Collaborator

That's definitely an area that could be improved. For now, I've kicked off CI for this PR. You should be able to commit additional changes and allow CI to run here.

It appears that CI has started failing, so for the purposes of this PR please disregard the existing failures while working on new tests. Thanks!

@mgorven mgorven force-pushed the view-replace branch 2 times, most recently from 6816617 to c852176 Compare June 9, 2023 23:25
@mgorven mgorven force-pushed the view-replace branch 4 times, most recently from b7c1db9 to 1d6e0d7 Compare June 26, 2023 19:18
@mgorven
Copy link
Contributor Author

mgorven commented Jun 26, 2023

As was discovered in #621, the tests don't actually create views (they create a table). I've changed the tests to run metadata.create_all() instead, which should create the view and backing tables.

@mgorven mgorven force-pushed the view-replace branch 2 times, most recently from f10afbe to c6fd76e Compare June 26, 2023 22:27
@mgorven
Copy link
Contributor Author

mgorven commented Jun 27, 2023

Only test failures are TestAggregatesWithManyToManyRelationships, which are failing in master.

Copy link
Collaborator

@kurtmckee kurtmckee left a comment

Choose a reason for hiding this comment

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

One last thing that I finally realized: the replace parameter needs a bit of documentation in the create_view() docstring.

It'll be rendered automatically the next time the docs are built, and will appear like the other parameters in the create_view() documentation.

sqlalchemy_utils/view.py Show resolved Hide resolved
@kurtmckee kurtmckee merged commit db32722 into kvesteri:master Jun 29, 2023
@kurtmckee
Copy link
Collaborator

Thanks for your work on this, @mgorven!

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.

2 participants