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. |
| ) | ||
|
|
||
| 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)
|
hi! sorry for the delays, I'll do one more review and merge it for you. |
totallyzen
left a comment
totallyzen
left a comment
There was a problem hiding this comment.
small window for error: MYSQL_DIALECT can still be mysql+something and it will break tests. But I think it's good enough like this. I'll merge and release this for you and you can raise another patch for the bug above
🤖 I have created a release *beep* *boop* --- ## [4.9.1](testcontainers-v4.9.0...testcontainers-v4.9.1) (2025-01-21) ### Bug Fixes * milvus healthcheck: use correct requests errors ([#759](#759)) ([78b137c](78b137c)) * **mysql:** add dialect parameter instead of hardcoded mysql dialect ([#739](#739)) ([8d77bd3](8d77bd3)) * **tests:** replace dind-test direct docker usage with sdk ([#750](#750)) ([ace2a7d](ace2a7d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
3 participants
closes #727
dialect;