v4: Refactor #new and #connect#596
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the TinyTds::Client initialization and connection handling by separating the #new and #connect methods and modernizing the API to use keyword arguments.
- Switched from hash-based options to keyword arguments for
TinyTds::Client.new - Separated connection logic from initialization -
#connectis now called automatically before SQL execution - Implemented GVL (Global Virtual Machine Lock) handoff during connection operations for better multi-threading performance
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/tiny_tds/client.rb | Refactored constructor to use keyword arguments and separated connection logic |
| ext/tiny_tds/client.c | Updated C extension to support automatic connection before SQL execution and GVL handoff |
| test/test_helper.rb | Updated test helper to use new keyword argument syntax |
| test/client_test.rb | Updated tests to reflect new connection behavior and API changes |
| test/result_test.rb | Minor test adjustment for new connection state handling |
| README.md | Updated documentation to reflect keyword arguments and renamed appname to app_name |
| CHANGELOG.md | Added changelog entries for the breaking changes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (azure == Qtrue || contained == Qtrue) { | ||
| #ifdef DBSETLDBNAME | ||
| DBSETLDBNAME(cwrap->login, StringValueCStr(database)); |
There was a problem hiding this comment.
The removal of the #ifdef DBSETLDBNAME guard makes this code dependent on a specific FreeTDS version without fallback handling. Consider adding version checks or error handling for systems where DBSETLDBNAME is not available.
CHANGELOG.md
Outdated
| * `TinyTds::Client.new` now accepts keyword arguments instead of a hash | ||
| * Renamed `tds_version` and `tds_version_info` to `server_version` and `server_version_info` | ||
| * Separate `#new` and `#connect` | ||
| * Instead, before running `#do`, `#execute` or `#insert`, `tiny_tds` will check if the connection is active and re-connect if needed.+ |
There was a problem hiding this comment.
There's a trailing '+' character at the end of the line that should be removed.
| * Instead, before running `#do`, `#execute` or `#insert`, `tiny_tds` will check if the connection is active and re-connect if needed.+ | |
| * Instead, before running `#do`, `#execute` or `#insert`, `tiny_tds` will check if the connection is active and re-connect if needed. |
andyundso
force-pushed
the
separate-new-and-connect
branch
from
cc06286 to
5a10d48
Compare
|
Looks like this introduces an issue with the will need to setup a development environment on Windows to see what is going on. |
andyundso
force-pushed
the
separate-new-and-connect
branch
from
57b9804 to
6e662e7
Compare
|
I think I see it. I'll also have to do (both |
`encoding` denotes the actual `Encoding` then used to associate strings.
andyundso
force-pushed
the
separate-new-and-connect
branch
from
6e662e7 to
321c66a
Compare
|
I've removed handing off the GVL for now from this branch and will look into it with separate PRs to implement it step-by-step (first |
1 participant
This PR refactors a few aspects around
#newand#connecton theTinyTds::Client.First, use keyword arguments instead of an
optshash. the list of arguments is quite long, but it allows users to take advantage of pattern matching when passing arguments to initialising a new client. The values are also preserved as instance variables in theTinyTds::Client, allowing to implement the second point of this PR.Second, I removed calling
#connectfrom#new. Instead,#connectis called from either#do,#insertor#executeshortly before the SQL is sent to the server (usingactive?, means a reconnect also happens automatically if the FreeTDSDBPROCESSdied). I originally wanted that#connecthas to be called manually, but from a usage-perspective, this is rather cumbersome, especially when we know best when#connecthas to be called.