Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make FIELDS_TO_CHECK uses casual field name in case of ForeignKey #136

Merged
merged 1 commit into from Aug 9, 2019

Conversation

@MounirMesselmeni
Copy link
Contributor

commented Oct 11, 2018

It's really confusing when specifying FIELDS_TO_CHECK to put your FK field name _id inside of it.

Make FIELDS_TO_CHECK uses casual field name in case of ForeignKey ins…
…tead of fieldname_id pattern which is confusing
@coveralls

This comment has been minimized.

Copy link

commented Oct 11, 2018

Coverage Status

Coverage remained the same at 95.556% when pulling 8c7e04b on MounirMesselmeni:master into 7c8d2fd on romgar:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

commented Oct 11, 2018

Coverage Status

Coverage remained the same at 95.556% when pulling 8c7e04b on MounirMesselmeni:master into 7c8d2fd on romgar:master.

@MounirMesselmeni

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

@romgar Any chance to review this?

@romgar

This comment has been minimized.

Copy link
Owner

commented Nov 19, 2018

Hey @MounirMesselmeni. That's a backward incompatible change. I think supporting the field definition without _id prefix makes sense, but I would then like to support both with and without _id.

@@ -42,7 +42,7 @@ def _as_dict(self, check_relationship, include_primary_key=True):
deferred_fields = self.get_deferred_fields()

for field in self._meta.fields:
if self.FIELDS_TO_CHECK and (field.get_attname() not in self.FIELDS_TO_CHECK):

This comment has been minimized.

Copy link
@romgar

romgar Mar 2, 2019

Owner

@MounirMesselmeni as explained in the discussion on this PR, can we make it backward compatible by allowing both with and without _id?
Thanks!

@trevoriancox

This comment has been minimized.

Copy link

commented Aug 8, 2019

I vote for a fix like this, or a documentation update. What's really unexpected is that currently, you have to specify fk_id in FIELDS_TO_CHECK, but then it will show up as fk (without _id) in the result of get_dirty_fields.

@romgar

This comment has been minimized.

Copy link
Owner

commented Aug 9, 2019

Yup, let's move this forward.

@romgar romgar merged commit 41c490f into romgar:master Aug 9, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 95.556%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.