Conversation
…med parameters Tests: Add tests to cover new feature with backward compatibility
|
Thank you for the contribution. |
src/crate/client/params.py
Outdated
| _NAMED_PARAM_RE = re.compile(r"%\((\w+)\)s") | ||
|
|
||
|
|
||
| def convert_named_to_positional( |
There was a problem hiding this comment.
I'd tend to move the method into the cursor module and prefix it with _.
It's not supposed to be public API and a dedicated module doesn't seem warranted for this tiny and simple function.
There was a problem hiding this comment.
Good call. I was thinking to isolate the regex logic in its own module to keep cursor.py focused on DB-API concerns and make the function independently testable.
I'm moving to _convert_named_to_positional and _NAMED_PARAM_RE directly into cursor.py at module level (not inside the class, since it doesn't touch self or any class state). And will delete params.py. What do you think about this structure?
There was a problem hiding this comment.
I'm moving to _convert_named_to_positional and _NAMED_PARAM_RE directly into cursor.py at module level (not inside the class, since it doesn't touch self or any class state). And will delete params.py. What do you think about this structure?
Sounds good
There was a problem hiding this comment.
Thank you for your review. I refactored the code based on that.
…module Docs: Update CHANGES.rst based on new implementation
Summary of the changes / Why this is an improvement
Adds client-side named parameter support (
pyformatparamstyle) to the CrateDB Python client, addressing the request in #774.Previously,
cursor.execute()only accepted positional?placeholders. Users with complex SQL had to maintain a positional list that was error-prone and hard to read.With this change, a
dictcan be passed as theparametersargument using%(name)splaceholders:The same parameter name may appear multiple times in the query, each occurrence is resolved independently:
Positional
?queries continue to work unchanged, no breaking change.Known limitation
Supporting named params in bulk operations are not supported because it requires a different approach from execute(). We need to extract the ordered parameter name list from the SQL. Excluded from this PR to keep this PR focused on the core execute() case.
Checklist