Conversation
Kulv3r
left a comment
Kulv3r
left a comment
There was a problem hiding this comment.
Nice persister!
Please fix these minor problems I mentioned and we can merge it.
| primary_key=self._primary_key) | ||
|
|
||
| @staticmethod | ||
| def __check_dependencies(): |
There was a problem hiding this comment.
Please look how it's implemented in the other persisters and adjust your code accordingly:
https://github.com/i2mint/py2store/blob/master/py2store/persisters/sql_w_sqlalchemy.py#L3
| import pyodbc | ||
|
|
||
| if 'ubuntu' in platform.platform().lower(): | ||
| result = subprocess.Popen(["dpkg", "-s", "msodbcsql17"], stdout=subprocess.PIPE, stderr=subprocess.PIPE) |
There was a problem hiding this comment.
This code will be executed every time you init a new SQLServerPersister instance.
There was a problem hiding this comment.
This is the intended behaviour, As py2store will be used for every storage type, but whenever user wants to use to for the SQLServer and creates an instance of the SQLServerPersister, we should check for dependencies of the SQLServer.
| self._cursor.execute(self._select_query.format(value=k)) | ||
| record = self._cursor.fetchone() | ||
| return record if record else print(f"No record found for primary_key: {k}") | ||
| # TODO: Raise a proper exception here |
There was a problem hiding this comment.
Please raise KeyError here and below.
| .format(conn_protocol, host, port, db_name, db_username, db_pass)) | ||
|
|
||
| self._cursor = self._sql_server_client.cursor() | ||
| self._table_name = table_name |
There was a problem hiding this comment.
We need to make sure this table will be created, if it's not present, so user dont have to worry about it.
There was a problem hiding this comment.
|
|
||
| class SQLServerPersister(MutableMapping): | ||
| def __init__(self, conn_protocol='tcp', host='localhost', port='1433', db_username='SA', db_pass='Admin123x', | ||
| db_name='py2store', table_name='person', primary_key='id', data_fields=('name',)): |
There was a problem hiding this comment.
primary_key might be a tuple of multiple key fields. Persister should be flexible enough to work with such cases.
Also, please rename it to key_fields.
hamidmujtaba
force-pushed
the
master
branch
2 times, most recently
from
30b2311 to
ad02e6e
Compare
Thanks for your kind review @Kulv3r, I've incorporated the changes you suggested. Please take a look. |
2 participants
Baseline SQLServer_Persister Added.
Added useful error messages while resolution of dependencies in order to run it.