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

[Feature] add 'create_if_not_exists' for views #599

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

eterna2
Copy link

@eterna2 eterna2 commented Apr 24, 2022

Feature:

Add flag create_if_not_exists for create_view and create_materialized_view.

Background:

Hi, I am using create_view and create_materialized_view to generate views via cli. However, i will like the cli to not throw an error if the views already exists. This PR adds a kwargs create_if_not_exists for create_view and create_materialized_view, which will insert CREATE VIEW {} IF NOT EXISTS if the flag is set.

The default for the flag is False so that everything will be backward compatible.

Add flag `create_if_not_exists` for `create_view` and `create_materialized_view`.
@eterna2
Copy link
Author

eterna2 commented Apr 26, 2022

@kurtmckee Not sure if you are the right person. Can u help me have a look at this PR?

@kurtmckee
Copy link
Collaborator

@eterna2, I don't feel qualified yet to review feature requests; my focus has been ticket triage and bug fixes. The project policy is to have unit tests associated with pull requests, so you'll need to add unit tests that demonstrate that this feature is working.

For example, it will be good to confirm that the function continues to raise an exception when the view already exists (the tests may already check this but you should confirm), and also test that the error is suppressed when create_if_not_exists is True.

@kvesteri, would you review this PR? I'm interested in your thoughts on the parameter name "create_if_not_exists". The implementation seems similar to how "materialized" is supported, but the parameter name seems inaccurate to my eyes and I don't have alternate suggestions.

sqlalchemy_utils/view.py Outdated Show resolved Hide resolved
sqlalchemy_utils/view.py Outdated Show resolved Hide resolved
sqlalchemy_utils/view.py Outdated Show resolved Hide resolved
@eterna2
Copy link
Author

eterna2 commented Apr 28, 2022

Hi, I had changed the kwargs to if_not_exists as requested.

Also made several other changes:

  • added kwargs or_replace for CREATE [OR REPLACE|ALTER] [MATERIALIZED] VIEW
  • added compilation for mysql, postgres, mssql, and snowflake dialects
  • updated flake8 rules specifically for view.py to ignore the long docstr and redefinitions for the dialect specific compilation.

I am actually using this for snowflake specifically.

@eterna2
Copy link
Author

eterna2 commented Apr 28, 2022

Sorry. Some unnecessarily format changes cuz i was using black to quickly do the autoformatting.

@kurtmckee
Copy link
Collaborator

Thanks @eterna2! I've kicked off the CI runs. Please also add unit tests that demonstrate the new functionality.

@eterna2
Copy link
Author

eterna2 commented May 5, 2022

Hi, I have added the unit tests for the new params. I just wrote the test for the generic use case and postgres.

@eterna2
Copy link
Author

eterna2 commented May 14, 2022

Seem like crypto package is raising a deprecation warning. Do u want me to add a marker to the requirement for py3.6?

@kurtmckee
Copy link
Collaborator

No, I can add that separately.

sqlalchemy_utils/view.py Outdated Show resolved Hide resolved
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.

3 participants