(improvement) remove protocol_version where possible #642
Conversation
In most of the cases, it is simply not needed. For the very few cases it is, kept it. I think it simplifies the code. Signed-off-by: Yaniv Kaul <[email protected]>
|
(Mostly done with AI, so blame it if it doesn't work) |
The Cython implementation in row_parser.pyx expects recv_results_rows() to take protocol_version as a parameter. Updated both the call site and method signature in protocol.py to match. This fixes the "recv_results_rows() takes exactly 6 positional arguments (5 given)" error that was causing authentication and cluster test failures.
OrderedMapSerializedKey now takes a single cass_type_instance argument instead of (cass_type, protocol_version). Updated the Cython deserializer to instantiate the key_type with protocol_version before passing it. This fixes the "map<varchar, varchar>: __init__() takes exactly 2 positional arguments (3 given)" error.
Changed desc.protocol_version to deserializer.protocol_version since protocol_version was removed from ParseDesc as part of the refactoring. The protocol_version is now stored in the Deserializer instances.
Updated test files to use instantiated types with protocol_version: - test_custom_protocol_handler.py: Changed ctype.from_binary(val, protocol_version) to ctype(protocol_version).from_binary(val) - test_loadbalancingpolicies.py: Changed Int32Type.serialize(i, protocol_version) to Int32Type(protocol_version).serialize(i) These tests were still using the old API where protocol_version was passed as a parameter rather than using type instances.
|
Finally looks like we are with the same CI failures that are unrelated to this patch and are failing in other PRs as well. |
Instead of setting msg.protocol_version externally and having send_body() access it as an instance variable, revert to the more explicit pattern of passing protocol_version as a parameter to send_body(). This makes the dependency more explicit and easier to understand. Changes: - Updated all send_body(self, f) methods to send_body(self, f, protocol_version) - Updated _write_query_params to accept and use protocol_version parameter - Updated encode_message to pass protocol_version to send_body instead of setting it on message - Updated all test files to pass protocol_version parameter to send_body - Rebuilt C extensions to reflect the changes
- test_marshalling.py: Extract DecimalType(proto_ver) to avoid duplicate calls - test_orderedmap.py: Extract key_type_class(protocol_version) to reuse instance - test_geometry.py: Extract cql_type(proto_ver) and PointType(0) to avoid duplicates This makes the code more readable and efficient by avoiding repeated instantiation of the same type objects.
|
I do like the changes that remove the remnants of protocol v3, like some of the changes in I don't however like the general removal of Recently in Rust Driver we had to make some ugly changes to add such parameter in some places, because it was not added preemptively. This strongly convinced me that it should be present in all places that depend on the protocol. |
Thanks, I'll do just the smaller change - the rest was mainly me playing with AI work and interacting with it, not for real value - thus far. I'm hoping to get more out of it than just refactoring with little value. |
2 participants
In most of the cases, it is simply not needed. For the very few cases it is, kept it. I think it simplifies the code.
CoPi did most of the work, I've looked after the horrors.
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.