-
Notifications
You must be signed in to change notification settings - Fork 320
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 database_exists
for MS SQL natively (fixes #411)
#414
base: master
Are you sure you want to change the base?
Conversation
Before merging this needs unit tests. |
@kvesteri it might already been tested by existing code, but without test coverage it is hard to say for sure. |
The unit tests are not covering MSSQL at the moment. Separate test should be added for this and setup of the travis should be changed to initialize a MSSQL database. |
The tests in https://github.com/kvesteri/sqlalchemy-utils/blob/master/tests/functions/test_database.py do not currently include tests for MSSQL. Thus as part this PR those should be added. |
@kvesteri even if there are no MSSQL specific tests, if MSSQL is set as a default database, the test that cover the essential functionality should cover it. No? |
@abitrolly no unfortunately it doesn't work like that currently. Ideally we would have a test matrix that executes all tests to various different database / sqlalchemy version combination setups. |
Codecov Report
@@ Coverage Diff @@
## master #414 +/- ##
=========================================
Coverage ? 90.99%
=========================================
Files ? 63
Lines ? 3209
Branches ? 0
=========================================
Hits ? 2920
Misses ? 289
Partials ? 0 Continue to review full report at Codecov.
|
From https://app.codecov.io/gh/kvesteri/sqlalchemy-utils/blob/793ff8ea5151c75e06f77fcce1e0fa6e3ab84b26/sqlalchemy_utils/functions/database.py (had to login) I see that this part of the code is covered, but it is not clear by which tests. Need a better coverage report. Perhaps generate one that is directly uploaded to GitHub Pages branch. |
Removed Codecov as it gives less useful info than expected. |
Just to make it obvious, I am not going to invest more time into this PR as I don't deal with MS SQL anymore. Feel free to close, fork or add tests. |
@abitrolly Thanks for the quick response! @kvesteri I found a blog post for how to set up MSSQL in GitHub CI that may allow PR's like this to instantly benefit. I recommend keeping this PR open so @abitrolly's work isn't lost. |
Any chance this PR gets merged any time soon? |
No description provided.