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

Catch deferred attribute exception #367

Merged
merged 1 commit into from
Mar 29, 2019

Conversation

slurms
Copy link
Contributor

@slurms slurms commented Mar 29, 2019

Problem

Fixes #331.

Solution

Catch the AttributeError and return the underlying field value.

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).

@codecov
Copy link

codecov bot commented Mar 29, 2019

Codecov Report

Merging #367 into master will decrease coverage by 0.27%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #367      +/-   ##
==========================================
- Coverage   98.72%   98.45%   -0.28%     
==========================================
  Files           6        6              
  Lines         707      710       +3     
==========================================
+ Hits          698      699       +1     
- Misses          9       11       +2
Impacted Files Coverage Δ
model_utils/tracker.py 96.84% <50%> (-1.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4a252d...959786e. Read the comment docs.

@slurms slurms force-pushed the fix-deferred-attributes branch from 66d3113 to 959786e Compare March 29, 2019 05:38
@auvipy auvipy merged commit d557c42 into jazzband:master Mar 29, 2019
@jarekwg jarekwg mentioned this pull request May 1, 2019
1 task
@Atorich Atorich mentioned this pull request Aug 20, 2019
mthuurne added a commit to ProtixIT/django-model-utils that referenced this pull request Jun 12, 2024
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 added a commit to ProtixIT/django-model-utils that referenced this pull request Jun 13, 2024
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`.
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