Create new FieldTracker that works well with foreign keys

[treyhunner]

This will fix the issue mentioned in #43.

[treyhunner]

I'm not sure whether it's overkill to duplicate all FieldTracker tests for ModelTracker. At the moment inheritance is used at least, but the models are just copy-pasted.

[carljm]

I think this should be named something like get_field_value instead, since that's what it returns, not a Field object.

[carljm]

Looks good! Made a few comments.

Regarding the tests, I would probably be inclined to cut out all the ModelTracker tests except for the one test necessary to gain code coverage of its set_field_map() method (and demonstrate how its behavior differs from FieldTracker). I don't feel strongly about it though; all its tests will be removed anyway in a few releases when we remove it.

Also, this PR will need documentation updates before it's merged. I'd handle the docs similarly to the code; change the main documentation to refer entirely to FieldTracker; at the end of the section have a note with a brief mention of ModelTracker, how its behavior differs, and the fact that it's deprecated and shouldn't be used in new code.

[treyhunner]

ModelTracker and FieldTracker now differ subtly for both changed and has_changed methods (as well as current and previous for foreign key fields).

@carljm I believe I've taken all of your feedback so far into account at this point.

Still to do:

• update documentation to refer to FieldTracker
• mention the deprecated ModelTracker and explain how it differs
• update CHANGES file to note new FieldTracker and deprecated ModelTracker
• add test for tracking non-field attributes with FieldTracker

[treyhunner]

This is a little odd to me, but I'm not sure how we could fix this in a general way.

What's going on here:

# default values
assert self.instance.name == ''
assert self.instance.number is None

# previous values default to None for new instance
assert self.instance.tracker.previous('name') is None
assert self.instance.tracker.previous('number') is None

# '' != None
assert self.instance.tracker.has_changed('name')
#  None == None
assert not self.instance.tracker.has_changed('number')

[carljm]

I think this is actually fine. I think the most important thing is to be clear about what the invariants are, and be consistent: e.g. that an unsaved instance is considered to have None for all previous() field values, and has_changed simply compares previous() with the current attribute value.

[carljm]

Looking good! A couple quick notes on the tests; these don't need to block this PR (especially since they pre-date it), but things to consider in the future:

• For things which should only be tested on a specific Django version, it's better to use real test-skipping (the skipIf decorator and friends from unittest2) than if statements in tests. This is possible now that we no longer support Django 1.2.
• For methods which test equality with a data dict (e.g. assertChanged) it's better to explicitly pass in a single dict than to exploit **kwargs, for several reasons: (1) the test gives a clearer visual picture of the data structure its asserting against, and (2) you don't run into smelliness when you need additional optional args (like tracker). Popping arbitrary args out of a **kwargs that is otherwise being treated as an opaque data container is a namespace-invasion that's a recipe for future confusion. It's not worth the minimal savings in typing in the tests.

[treyhunner]

For things which should only be tested on a specific Django version, it's better to use real test-skipping (the skipIf decorator and friends from unittest2) than if statements in tests. This is possible now that we no longer support Django 1.2.

I agree. The Django 1.5 check should be refactored into a separate test with a skipIf.

For methods which test equality with a data dict (e.g. assertChanged) it's better to explicitly pass in a single dict than to exploit *_kwargs, for several reasons: (1) the test gives a clearer visual picture of the data structure its asserting against, and (2) you don't run into smelliness when you need additional optional args (like tracker). Popping arbitrary args out of a *_kwargs that is otherwise being treated as an opaque data container is a namespace-invasion that's a recipe for future confusion. It's not worth the minimal savings in typing in the tests.

I agree. This is especially confusing in the current tests because I've used **kwargs for multiple field-based assertions in a single method (good imo) and for comparison to a dict (bad). I definitely think the latter should be changed from **kwargs to a dict.

[carljm]

Odd, the comments about get_field_map seem to have disappeared.

I hadn't thought of the tracking-non-fields use case. If you add a test for that, I'm fine with your implementation of get_field_map - now I understand why it's more complicated than I thought it needed to be :-)

[carljm]

Besides the minor doc additions I just mentioned, is this waiting for anything else?

[treyhunner]

Feel free to add both of those notes to the branch if you have a particular
wording in mind. I will review the current tests later today to ensure the
coverage is appropriate before we merge.
On Jun 3, 2013 10:32 AM, "Carl Meyer" notifications@github.com wrote:

Besides the minor doc additions I just mentioned, is this waiting for
anything else?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/52#issuecomment-18857199
.

[carljm]

Ok, doc updates pushed. If you're willing, please review my summary of the differences between ModelTracker and FieldTracker to ensure I didn't miss anything or get any subtleties wrong.

[treyhunner]

The differences listed in the README note look good to me.

I just added the missing tests for tracking a non-field model attribute. Those tests fail right now because round(8.5) == 8 in Python 3. After I fix the tests everything should be ready to merge.

[treyhunner]

I fixed the rounding issue. This should be good to merge now.