Django 1.11#2206
Merged
sphuber merged 20 commits intoNov 16, 2018
Merged
Conversation
+ remove django-extensions
In the django backend, we were using the UUIDField of django-extensions (the only use of django-extensions in the codebase). Since django now provides a UUIDField, django-extensions discontinued it and we have to move as well. There are a few changes: * the old UUIDField had an "auto" keyword which resulted in a uuid being automatically generated if none was provided at creation. In the new UUIDField, this should be the default behavior (?) * the old UUIDField had a "version" keyword which allowed to select the UUID version. The new UUIDField instead has a "default" keyword, which allows to provide a function to set the default value For reference, this is the code of the old UUIDField in django-extensions: https://github.com/django-extensions/django-extensions/blob/cda632d6f5bd7ac7c63e2172789a55cd21dedd7a/django_extensions/db/fields/__init__.py#L371 And this is the new UUIDField in django 1.11: https://github.com/django/django/blob/c7cc7526d5ee7d38a6ee1af03610f1aba1ea0c78/django/db/models/fields/__init__.py#L2291 * this has no "auto" keyword (id=None should cause to use the 'default' when saving) * https://stackoverflow.com/a/35210896 * the default is now "uuid.uuid4()" * this is the version we use in AiiDA, so we could just remove the "version" arg (or we could provide our own UUIDField, where we hardcode version 4 (in case django changes their default in the future) *
aiida/backends/djsite/settings/settings.py contained a setting ROOT_URLCONF = 'aiida.backends.djsite.settings.urls' The problem: The file aiida.backends.djsite.settings.urls did not exist. django 1.11 tried to import this non-existing file. Simply removing this variable solved the problem.
since both sqla and django backends now use the 'uuid' column type for uuids, the get_filter_expr_from_column function becomes identical for the django and sqla querybuilder, and has been moved to the querybuilder interface.
UUIDs were used as keys of dictionaries
django: introduce uniqueness constraints on all uuid columns in the DB
convert uuid to string where it tests whether row already present in the DB.
__getitem__ of django 1.11 queryset was no longer using our iterator
Was used only in one test + there is a perfectly fine querybuilder-based implementation
…-1.11-merge-provenance-design
Member
Author
|
It's not too much code to review; happy to make changes where desired |
sphuber
approved these changes
Nov 16, 2018
Contributor
sphuber
left a comment
sphuber
left a comment
There was a problem hiding this comment.
Minor comment and a question, but other than that looks good
|
|
||
| :returns: An instance of sqlalchemy.sql.elements.BinaryExpression | ||
| """ | ||
| # Label is used because it is what is returned for the |
Contributor
There was a problem hiding this comment.
I think this comment is outdated, because it refers to the old DbCalcState table that has been removed. The state is now a regular attribute. This might make the conditional below easier as we can remove Label but I am not sure.
| pass | ||
|
|
||
| @classmethod | ||
| def get_filter_expr_from_column(cls, operator, value, column): |
Contributor
There was a problem hiding this comment.
I notice this has SqlAlchemy imports but is moved to the abstract class. Is this intentional? Because of how the querybuilder for django actually also operates on SqlAlchemy in some way?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: for the moment, I've replaced the uuid fields also in old migrations. One could try to use a
CharField(editable=False, blank=True, max_length=36)there to be safeget_filter_expr_from_columnfrom django/sqla to interfaceget_all_parentsfunction (was only used in tests)