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

Remove catching of AttributeError in DescriptorWrapper #624

Merged
merged 2 commits into from
Jun 13, 2024

Conversation

mthuurne
Copy link
Contributor

@mthuurne mthuurne commented Jun 12, 2024

Problem

I was cleaning up the type annotations in DescriptorWrapper.__get__():

    def __get__(self, instance: models.Model | None, owner: type[models.Model]) -> DescriptorWrapper[T] | T:
        if instance is None:
            return self
        was_deferred = self.field_name in instance.get_deferred_fields()
        try:
            value = self.descriptor.__get__(instance, owner)
        except AttributeError:
            value = self.descriptor
        if was_deferred:
            tracker_instance = getattr(instance, self.tracker_attname)
            tracker_instance.saved_data[self.field_name] = lightweight_deepcopy(value)
        return value

After being puzzled by the code for some time, I realized that the value = self.descriptor statement was inherently incompatible with the return type of the method. Also storing the descriptor rather than a value in tracker_instance.saved_data seems unlikely to be correct.

Solution

I tried to find out when this exception handler runs, but it turned out it is never executed during unit testing.

Digging deeper, I found that AbstractModelTrackerTests was supposed to cover this code, but that test suite was marked as skip because it had known test failures. Re-enabling it indeed led to 4 failing test cases, all from ModelTrackerTests. The reason for that was simple though: the test model TrackedAbstract used a FieldTracker rather than a ModelTracker. After fixing that, the test suite passed.

There used to be a bug in Django that caused these tests to fail, according to the discussion of #370. That bug was fixed in Django 4.0.

This exception handler was introduced in #367, but subsequent discussion in #370, #381 and #382 suggests that it wasn't the proper solution. As we now have a working test and that test doesn't cause AttributeError to be raised, I think it is safe to remove the try/except.

Commandments

  • Write PEP8 compliant code.
  • Cover it with tests.
  • Update CHANGES.rst file to describe the changes, and quote according issue with GH-<issue_number>.
  • Pay attention to backward compatibility, or if it breaks it, explain why.
  • Update documentation (if relevant).

This change should be invisible to end users, as it removes dead code. So I think the ChangeLog and docs steps can be skipped, but if you disagree, let me know and I'll update the PR.

mthuurne added 2 commits June 12, 2024 16:54
There used to be a [bug in Django](https://code.djangoproject.com/ticket/30427)
that caused these tests to fail, according to the discussion of jazzband#370.
That bug was fixed in Django 4.0.

However, because the test model was using a `FieldTracker`
rather than a `ModelTracker`, all model-specific test cases
in `ModelTrackerTests` continued to fail.
This catch was introduced in jazzband#367, but subsequent discussion
in jazzband#370, jazzband#381 and jazzband#382 suggests that it wasn't the proper solution.

Now that `AbstractModelTrackerTests` is re-enabled and passing,
it becomes clear that `AttributeError` is no longer raised during
unit testing when using a recent Django version. Therefore I think
it is safe to remove the `try`/`except`.
@mthuurne mthuurne mentioned this pull request Jun 12, 2024
@foarsitter
Copy link
Contributor

Underlines how valuable typing can be! Thanks for your thorough investigation.

@foarsitter foarsitter merged commit e35c724 into jazzband:master Jun 13, 2024
7 checks passed
@mthuurne mthuurne deleted the DescriptorWrapper-no-catch branch June 13, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants