Conversation
sqeleton/databases/mssql.py
Outdated
| return table("information_schema", "tables") | ||
|
|
||
| def list_tables(self, table_schema: str, like: Compilable = None) -> Select: | ||
| if table_schema == None: |
There was a problem hiding this comment.
You can make this a lot shorter by using table_schema or SKIP
There was a problem hiding this comment.
Yep, good catch. I have implemented a change.
| # Subtracting 2 due to wierd precision issues in PostgreSQL | ||
| return super()._convert_db_precision_to_digits(p) - 2 | ||
|
|
||
| def set_timezone_to_utc(self) -> str: |
There was a problem hiding this comment.
Is it always UTC? If so, worth documenting.
There was a problem hiding this comment.
It is, I have added code commentary. Is there an additional location that this should be documented?
sqeleton/databases/mssql.py
Outdated
| bool: "BIT", | ||
| datetime: "datetime2", | ||
| }[t] | ||
| except: |
erezsh
left a comment
erezsh
left a comment
There was a problem hiding this comment.
Overall looks good!
Left a few small comments.
sqeleton/databases/mssql.py
Outdated
| return super()._query_cursor(c, sql_code) | ||
| except self.mssql.DatabaseError as e: | ||
| raise QueryError(e) | ||
| except e: |
There was a problem hiding this comment.
This is unnecessary, and actually will hide the real type of the exception.
There was a problem hiding this comment.
I think I added this originally to match the oracle module. I have now removed this.
sqeleton/databases/mssql.py
Outdated
| return f"datetimeoffset" | ||
| try: | ||
| return { | ||
| str: "VARCHAR(1024)", |
There was a problem hiding this comment.
Sorry, I'm not well-versed in mssql. Is it a good idea to specify 1024? Is that the maximum size?
There was a problem hiding this comment.
I have have changed this to used NVARCHAR(MAX) to better align with other database modules. I also noticed an issue relating to foreign keys that I have now remedied.
pyproject.toml
Outdated
| [tool.poetry] | ||
| name = "sqeleton" | ||
| version = "0.1.7" | ||
| version = "0.1.8" |
There was a problem hiding this comment.
Please don't change the version. I will do it later when I make a release.
(maybe you changed it for testing and forgot to remove?)
There was a problem hiding this comment.
Yes, sorry I was testing the difference between the versions. I have now reverted this.
2 participants
Added MsSQL database entry and included test coverage that matches current pattern for other supported databases.