Add __copy__, __deepcopy__ to URL. Fixes: #7400#7401
Add __copy__, __deepcopy__ to URL. Fixes: #7400#7401rec wants to merge 1 commit intosqlalchemy:mainfrom
Conversation
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision a2c1b89 of this pull request into gerrit so we can run tests and reviews and stuff
|
New Gerrit review created for change a2c1b89: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3352 |
zzzeek
left a comment
zzzeek
left a comment
There was a problem hiding this comment.
great, thanks.
I want to cherry-pick this to 1.4, add a changelog, and also update it a bit, which I can do on this end very easily from the Gerrit side.
| is_false(url1 == url3) | ||
|
|
||
| def test_warnings(self): | ||
| assert_raises( |
There was a problem hiding this comment.
we should have tests for this elsewhere so I will take this part out.
| url1 = url.URL.create("dbtype") | ||
| url2 = copy.copy(url1) | ||
| url3 = copy.deepcopy(url2) | ||
| is_true(url1 == url2) |
There was a problem hiding this comment.
we have a fixture eq_() for this, eq_(url1, url2). it's not any better or worse we just try to be consistent.
| ) | ||
|
|
||
| def __deepcopy__(self, memo): | ||
| return self.__copy__() |
There was a problem hiding this comment.
self.query is a dict so we likely need to deepcopy that also
There was a problem hiding this comment.
though if it's a dict of str->str in all cases, then not. let me check that
There was a problem hiding this comment.
Ooops!
It still needs to be copied, though, you don't want the same mutable dict in two entries.
There was a problem hiding this comment.
it's fine the way it is, the dict is immutable (special SQLAlhcemy class) and the contents are all strings / tuples of strings / tuples . fully immutable contents as well. thanks !
There was a problem hiding this comment.
Immutable dicts, great idea!
Thanks again for an excellent package, and I hope the rest of your weekend is fantastic.
|
Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3352 has been merged. Congratulations! :) |
|
Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3353 has been merged. Congratulations! :) |
|
mike bayer (zzzeek) wrote: thanks for the review! View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3352 |
3 participants
Description
Allows
copy.copyandcopy.deepcopyto be used on instances ofsqlalchemy.engine.url.URL.Checklist
This pull request is:
must include a complete example of the issue. one line code fixes without an
issue and demonstration will not be accepted.
Fixes: #<issue number>in the commit message