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 for SQL Server #544

Closed
wants to merge 7 commits into from

Conversation

tarsil
Copy link
Contributor

@tarsil tarsil commented Mar 28, 2023

@aminalaee @Kludex

This is a PR proposing support for SQL Server. A lot of companies still use it and with aioodbc and databases patterns, this was possible to achieve as well. This could be merged maybe after this one? The reason why after its only because I branched off that branch to create this new one and once it's merged, it is way less files to review.

* Removed support for python 3.7
* Added common and dialects packages to handle
the new SQLAlchemy 2.0+
* The connections were hanging sometimes
during the tests.
* Revert changes automatically applied by IDE
* Fix formatting of CI caused by IDE changes
* Codebase cleanup for SQLAlchemy 2.0
* Fix EOL in mkdocs
@rafalp
Copy link
Member

rafalp commented Mar 28, 2023

Do we want to extend Databases with new backends when library is basically on life support? 🤔

@tarsil
Copy link
Contributor Author

tarsil commented Mar 28, 2023

@rafalp the lib is actually very stable and works for 99% of the projects as is. With the latest 2PR being reviewed we go to the next level. Unless @aminalaee and @Kludex feel that Encode doesn't want to continue with this. If that is the case, there is always a way to continue anyway :) but I believe Encode wants to keep maintaining this :)

@rafalp
Copy link
Member

rafalp commented Mar 28, 2023

I believe Encode wants to keep maintaining this

I'm also Encode member and I was on meetings where we've discussed how to carry on with databases with my proposition being to keep it around because its great as lightweight async DBAL alternative to solve-it-all SQlAlchemy 2 ;)

I think we should update the readme to make it more obvious that the plan is to keep the library around for a while, especially when people are seeing PRs removing mentions of Databases from Starlette's docs and asking questions.

@tarsil
Copy link
Contributor Author

tarsil commented Mar 28, 2023

@rafalp I didn't mean any disrespect about you ahahah. I respect Encode tremendously :D. A lot of what I do is inspired by the great work you all do.

I have more than interest to use databases and the reason why I "created" the driver for SQL Server is because I'm currently working on projects that use heavily SQL Server and the amount of code being used there is so ugly that databases solves it in a simple fashion. I never wanted to fork databases in order to put SQLAlchemy 2 and that is why I tried to open the PR here with the migration working but I also believe Encode knows what is the best course of action and I will always support the decisions 👍🏼

If you are Encode, I'm sure you are a fantastic developer! :D

This PR althoug it looks big, it is mostly because I branched of the one implementing SQLAlchemy 2.0 but not yet merged.

If Encode really wants to stop maintaining Databases (which according to you, it doesn't look like it). I offer myself to continue with the package.

@rafalp
Copy link
Member

rafalp commented Mar 28, 2023

TBH I've always been thinking that Databases should support external drivers, so adding new databases to it should not changing the core library.

I'm not familiar with SQL Server but this PR seems to mix SQLAlchemy 2 support changes with SQL Server changes.

@tarsil
Copy link
Contributor Author

tarsil commented Mar 28, 2023

TBH I've always been thinking that Databases should support external drivers, so adding new databases to it should not changing the core library.

I'm not familiar with SQL Server but this PR seems to mix SQLAlchemy 2 support changes with SQL Server changes.

No, this is the same level of integration that is done for Postgres and MySQL but supporting SQLServer as well. SQL Alchemy has support for it already. What I'm proposing is simply to also add the same layer to MSSQL since it's widely used.

The PR it looks big beause the SQL Alchemy 2.0 changes were not yet merged. It is here. Because I wanted to use already SQLA 2.0, I branched off that branch and added the SQLServer driver. Once that PR is merged, if is merged, you should only see 3 or 4 files only here to review

@tarsil tarsil closed this Apr 6, 2023
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