fix(mysql): add dialect parameter instead of hardcoded mysql dialect #739
Conversation
|
@totallyzen hi! sorry for the unnecessary trouble but I fixed it. please look at the changes |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #739 +/- ##
=======================================
Coverage ? 84.77%
=======================================
Files ? 12
Lines ? 670
Branches ? 105
=======================================
Hits ? 568
Misses ? 79
Partials ? 23 ☔ View full report in Codecov by Sentry. |
| @@ -104,8 +106,9 @@ def _connect(self) -> None: | |||
| ) | |||
|
|
|||
| def get_connection_url(self) -> str: | |||
| dialect = "mysql" if self.dialect is None else f"mysql+{self.dialect}" | |||
There was a problem hiding this comment.
I think you should put this simply in the init when you set self.dialect.
It would effectively become:
self.dialect = dialect or environ.get("MYSQL_DIALECT") or "mysql"also I think dialect="mysql+asdf" makes more sense than dialect="asdf"
There was a problem hiding this comment.
moving to init is a good idea
but it is inconvenient for the user to create the string "mysql+asdf"
IMO dialect is a specific extension that comes after "mysql". Why repeat “mysql” everywhere if we already know that we are connecting to mysql?
besides, someone might make a typo
There was a problem hiding this comment.
@totallyzen Not that this is an argument, but it was interesting to look at the current implementation of the postgresql container, and it is done according to the principle that I proposed. what do you think?
There was a problem hiding this comment.
and it is done according to the principle that I proposed. what do you think?
sure I don't mind - maybe then validate the dialect does not start with mysql (raise a ValueError) then I'm happy :)
There was a problem hiding this comment.
ready to make this check what proposed @balint-backmaker if you approve it @totallyzen, fine?
There was a problem hiding this comment.
oops, sorry for the mix - commented from the company account and not my personal 😅
(balint-backmaker is the company account, I do not normally use that here)
There was a problem hiding this comment.
@totallyzen did it in last commit
There was a problem hiding this comment.
@totallyzen ping)
|
hi! sorry for the delays, I'll do one more review and merge it for you. |
closes #727
dialect;