Add sql server support #36
Conversation
|
Thanks for this addition, and sorry for the delay. This one got lost in my inbox and I did not see it until now. |
As for this error, you can add it to this list and it should go away: https://github.com/ahopkins/mayim/blob/main/pyproject.toml#L20
I can handle the documentation part 😎 |
| scheme = "mssql+pyodbc" | ||
|
|
||
| def __init__(self, db_path: str): | ||
| self._db_path = db_path |
There was a problem hiding this comment.
I feel like this sort of breaks the pattern. The pools already are meant to parse out these strings and assign to properties. This is so that we can safely output a DSN in logs without exposing secrets, etc. It feels like this is a reimplementation.
| async def open(self): | ||
| """Open connections to the pool""" | ||
| conn_string = self._parse_url() | ||
| self._db = pyodbc.connect(conn_string) |
There was a problem hiding this comment.
Why _db? On other interfaces this is mean to be the database schema being connected to. I think this should be _pool.
| if not self._db: | ||
| await self.open() | ||
|
|
||
| yield self._db |
There was a problem hiding this comment.
Is there no actual connection object that can be cached and reused?
| class SQLServerQuery(SQLQuery): | ||
| __slots__ = ("name", "text", "param_type") | ||
| PATTERN_POSITIONAL_PARAMETER = re.compile(r"\?") | ||
| PATTERN_KEYWORD_PARAMETER = re.compile(r"\:[a-z_][a-z0-9_]") | ||
|
|
||
| def __init__(self, name: str, text: str) -> None: | ||
| super().__init__(name, text) | ||
| positional_argument_exists = bool( | ||
| self.PATTERN_POSITIONAL_PARAMETER.search(self.text) | ||
| ) | ||
| keyword_argument_exists = bool( | ||
| self.PATTERN_KEYWORD_PARAMETER.search(self.text) | ||
| ) | ||
| if keyword_argument_exists: | ||
| raise MayimError("Only Positional arguments allowed in pyODBC") | ||
| if positional_argument_exists: | ||
| self.param_type = ParamType.POSITIONAL | ||
| else: | ||
| self.param_type = ParamType.NONE |
There was a problem hiding this comment.
Let's make sure to add some unit tests to this.
| ## SQL Server | ||
|
|
||
| Dependencies: | ||
| - [pyodbc](https://github.com/mkleehammer/pyodbc) |
Hi,
I would love to use Mayim with SQL Server, so I added an SQL-Server-Executor. It uses pyODBC, which could be used for other databases as well, so a more generic executor might have been better, but it was unclear to me how to handle different connection strings.
Right now it uses the same url scheme as SQLAlchemy does, e.g.
mssql+pyodbc://sa:[email protected]/databasename?driver=ODBC+Driver+17+for+SQL+Server. This should be documented as well, but I was unsure where exactly I should put it.Also on code checking I get this error:
It seems like pyODBC stubs are missing, I have no clue how to fix that though.
Perhaps you might want to have a look at the code and give me some tips to improve it, so it can get merged eventually.