chore: Fix save behavior for Django 3 and drop Django 2 support #89
Conversation
dshafer
force-pushed
the
chore/fix-saves
branch
from
83919b4 to
fc345f7
Compare
* fix BoundEndpointManager get_response saves * Remove unused code Co-authored-by: Robert David Grant <robert.grant@salesforce.com>
| if ( | ||
| ce.save_changes | ||
| and resource | ||
| and hasattr(resource, "is_dirty") |
There was a problem hiding this comment.
This was a missing hasattr which allows DDA to work with models that don't use DirtyFields
There was a problem hiding this comment.
(unrelated to Django 3)
| @@ -904,10 +940,10 @@ class ResourceEndpointDefinition(EndpointDefinition): | |||
| """ | |||
|
|
|||
| def __init__(self, *args, **kwargs): | |||
| super().__init__() | |||
| super().__init__(*args, **kwargs) | |||
There was a problem hiding this comment.
This is a fix noticed in passing (unrelated to Django 3)
…ative-apis into chore/fix-saves
| "{0} instance not found".format(self.type.__name__) | ||
| ) | ||
| if hasattr(self.type, "__name__"): | ||
| message = "{0} instance not found".format(self.type.__name__) |
There was a problem hiding this comment.
f-string would be preferred here, no? or are we concerned about preserving python backwards compatibility?
There was a problem hiding this comment.
I think we should try to support the versions of Python "in support", which is currently 3.7, 3.8, 3.9, 3.10.
https://www.python.org/downloads/
(so I don't care about old Python versions)
| raise errors.ClientErrorNotFound( | ||
| "{0} instance not found".format(self.type.__name__) | ||
| ) | ||
| if hasattr(self.type, "__name__"): |
There was a problem hiding this comment.
Pedantic: EAFP > LBYL (try/except over if/else)
|
|
||
|
|
||
| try: | ||
| import dirtyfields |
There was a problem hiding this comment.
What's this about? dirtyfields is a hard requirement, no?
There was a problem hiding this comment.
I was told that DDA supports models that use dirtyfields but doesn't require it. I don't know if that's true in practice.
Agreed. I would ideally like to support versions of Python and Django that are currently "supported" by the maintainers of those projects, but that doesn't include Python 3.6 or Django 2.2 anymore. |
| @@ -9,6 +9,11 @@ | |||
| import inspect | |||
| import types | |||
|
|
|||
| try: | |||
| from django.core.exceptions import FieldDoesNotExist | |||
There was a problem hiding this comment.
We can drop this too if we don't care about Django 2.2 compatibility:
https://docs.djangoproject.com/en/4.0/releases/3.1/#miscellaneous-1
There was a problem hiding this comment.
Might bring it up in slack, but 2.2 has been dropped from official support so I'm not keen on keeping it around.
…ative-apis into chore/fix-saves
bgrant
changed the title
| field_name_to_att_name = {f.name: f.attname for f in resource._meta.concrete_fields} | ||
| for k, v in resource_next._as_dict(check_relationship=True).items(): | ||
| att_key = field_name_to_att_name[k] | ||
| if getattr(resource, att_key, None) != v: |
There was a problem hiding this comment.
Why have the check? I guess it's a performance toss-up: Is it worth writing every field every time, which would likely be less performant in cases where only one field changes vs when many fields are updated.
There was a problem hiding this comment.
Yeah - this is @dshafer's code, but that's my guess.
| @@ -9,6 +9,11 @@ | |||
| import inspect | |||
| import types | |||
|
|
|||
| try: | |||
| from django.core.exceptions import FieldDoesNotExist | |||
There was a problem hiding this comment.
Might bring it up in slack, but 2.2 has been dropped from official support so I'm not keen on keeping it around.
Work related to this change in Django 3:
Django 3.0 release notes | Django documentation | Django