PYTHON-649#677
Conversation
| for user_type in set(udts): | ||
| user_type.register_for_keyspace(klass._get_keyspace()) | ||
| for user_type, conn in product(set(udts), connection._connections.values()): | ||
| user_type.register_for_keyspace(klass._get_keyspace(), connection=conn) |
There was a problem hiding this comment.
This would probably work. However, It might be better to keep the 'connection' logic only in the management commands. I think It will become confusing if we start adding some of that logic in different places. What do you think?
There was a problem hiding this comment.
I see what you mean -- let me have a look at what consolidating the logic would involve.
|
Pushed 5c649a9; rebasing for a cleaner history. |
c4f9989 to
ae28fb2
Compare
|
I've pushed a new test for behavior that was broken with #656, and a fix for that. I think there's definitely some code dedupe to be done here before merge. |
| default_name = 'default' | ||
| conn = Connection.from_session(default_name, s) | ||
| _connections[default_name] = conn | ||
| set_default_connection[default_name] |
There was a problem hiding this comment.
This doesn't seem to work as expected. I'm getting this exception:
connection.set_session(s)
File "/home/aboudreault/dev/python-driver/cassandra/cqlengine/connection.py", line 238, in set_session
set_default_connection[default_name]
TypeError: 'function' object has no attribute '__getitem__'
You probably want to do set_default_connection(...).
|
Fixed the Unit tests now pass locally. Do you agree that it's worth the time to refactor before we commit? I don't like the duplication between |
|
+1 I agree that set_session could be removed in the next major release. register_connection should be able to register a new connection from a session object. I don't have any objection if we can add that support now (if it's easy) but we need to keep set_session. You can add a note about the the removal of it in https://datastax-oss.atlassian.net/browse/PYTHON-504 |
b86ba06 to
70eddcf
Compare
|
Looks good. it might be good to add some documentation about that, with 2 examples, and also specifying what configuration arguments are incompatible with the session param. |
|
+1 |
This adds to the end of the argument list, so it doesn't change the API for clients who depended on argument order.
I don't necessarily expect this to get merged as-is; this is more of a review request. I want to check:
for user_type, conn in product(set(udts), connection._connections.values())approachtests/integration/cqlengine/model/test_udts.pycover the cases we care about, in terms of ordering UDT definitions and connection creations. I think they have, but I'd like a review and to have another look myself.