Alternative pks #192
Alternative pks #192
Conversation
Concerned about the bad form of reading `_state` from outside the object
Least sure about this change - not sure why this became needed, which worries me. Without it, depth was None causing a failed DB NOT NULL constraint failure
… into allowing-for-uuid-ids
strings are more widely correct as ints will be represented correctly, and it will allow using the `__str__` of any custom type used in the model id field
upstream updates
…o-treebeard into alternative-pks
…reebeard into alternative-pks
|
Hey, It includes all the tests of all three PRs. The Let me know if anyone needs something additional included in this PR. I am open to accepting patches. Alternatively, just add send in a PR to django-treebeard directly. @tabo @jrief @solarissmoke @kolanos @rhunwicks @obayemi @JamesMaroney |
|
Changes look good, but in the future it would be better to have PRs that are adhered to their description, because I see changes in testing infra, sphinx configuration, and a bump from sql server 2016 to 2019 (?!?). |
|
@tabo Sure, the reason for that is that the tests included testing for Python 3.9 and Python 3.9 is only supported by the latest version of the appveyor image, which in turn no longer supports mssql 2016, etc. . So one thing led to another and the only way I could get the Python 3.9 tests to run was to also upgrade everything else. I guess the main problem is just that there were several outdated things in there and this will not be a problem going forward as the package will get more frequent updates and releases. |
|
I just installed sql server 2016 last night so I could run the tests 😂 Good work here! And I agree with @rhunwicks about the usage of 'id', so let's get the other PR moving forward too. |
|
And to reiterate my point about changes to test infra, I understand the need for updating the test infrastructure and settings, but the main reason to do it separately is that things could be broken BEFORE this PR lands, and the extended tests could tell us that. Also, if the tests failed (which they didn't), by keeping the changes separate we could know for sure that failure didn't come from the testing environments. |
|
Yes, so the upgrade to python 3.9 was already part of one of the three PRs
I merged. The corresponding testing infrastructure was just missing.
Under normal circumstances, if we weren't working on a multi-year backlog,
and if I had written the PR by myself, I'd agree and have multiple atomic
single issue PRs.
So who is going to merge this? The other PR is also ready for review, but
that one may require a major version bump as this could break it for some
existing users. Especially renaming `keep_ids` to `keep_pks`. Not sure.
…On Sat, Jan 16, 2021, 01:08 Gustavo Picón ***@***.***> wrote:
And to reiterate my point about changes to test infra, I understand the
need for updating the test infrastructure and settings, but the main reason
to do it separately is that things could be broken BEFORE this PR lands,
and the extended tests could tell us that. Also, if the tests failed (which
they didn't), by keeping the changes separate we could know for sure that
failure didn't come from the testing environments.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#192 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAERMOFIZ6OUISOLZ5LIRD3S2DKHVANCNFSM4WEA6RHQ>
.
|
|
It's merged. |
|
Thanks @tabo ! We have tested and it is working for all our projects now. Any idea when we can see a release with this included? |
|
Yes !!! thank you a lot @johanneswilm @tabo I didn't have much hope it would get merged on day... Sorry I was locked out of my giithub account for a few days so I didn't get the notification, but you made a very good job! I'll migrate the project where I use my fork to upstream in a few days 👍 |
This is a combined PR of #30 #93 and #182. See discussion in #30 for details.