Opened 9 years ago

Closed 3 years ago

#6702 closed Cleanup/optimization (wontfix)

ModelForm: assert given instance is instance of model

Reported by: Thomas Güttler Owned by:
Component: Forms Version: master
Severity: Normal Keywords:
Cc: Selwin Ong, bmispelon@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Hi,

this assert statement can help to find bugs early. Patch attached.

Attachments (1)

newforms_models_assert_isinstance.diff (635 bytes) - added by Thomas Güttler 9 years ago.

Download all attachments as: .zip

Change History (16)

Changed 9 years ago by Thomas Güttler

comment:1 Changed 9 years ago by Thomas Güttler

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

BTW, all unittests pass

modw@berry:~/_djangosvn/trunk/tests> ./runtests.py  forms  2> o-old
... uncomment assert statement.
modw@berry:~/_djangosvn/trunk/tests> ./runtests.py  forms  2> o-new
diff o-old o-newmodw@berry:~/_djangosvn/trunk/tests> diff o-old o-new
88c88
< Ran 28 tests in 1.649s
---
> Ran 28 tests in 0.936s

comment:2 Changed 9 years ago by Bastian Kleineidam <calvin@…>

I don't like that asserts are removed when run with python -O. If a model instance is really required at this point, it would IMO be better to raise an exception.

comment:3 Changed 9 years ago by Malcolm Tredinnick

Triage Stage: UnreviewedAccepted

An assert is fine here. Asserts are the right control structure for checking that the programmer did what they were meant to do in any case, since it's not user-supplied data (which would require a non-vanishing error). Let's remember that this is Python: ask forgiveness, not permission and all that. Paranoid pre-execution type checking isn't the norm.

comment:4 Changed 8 years ago by Thomas Güttler

Needs tests: set

comment:5 Changed 7 years ago by Chris Beaven

Owner: changed from nobody to Chris Beaven
Status: newassigned

comment:6 Changed 7 years ago by Chris Beaven

Owner: Chris Beaven deleted
Status: assignednew

comment:7 Changed 5 years ago by Julien Phalip

Type: Cleanup/optimization

comment:8 Changed 5 years ago by Julien Phalip

Severity: Normal

comment:9 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:10 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:11 Changed 4 years ago by Selwin Ong

Cc: Selwin Ong added
Needs tests: unset

I opened a pull request on Github to fix this issue here: https://github.com/django/django/pull/296 . Test included.

comment:12 Changed 3 years ago by Baptiste Mispelon

Cc: bmispelon@… added
Needs documentation: set
Patch needs improvement: set

This is technically backwards-incompatible, so a mention in the release notes is warranted I think.

Also, I'd like to see some more tests involving abstract models, inherited models or proxy ones, just to be sure we're not breaking any valid use-cases.

Thanks.

comment:13 Changed 3 years ago by Thomas Güttler

I was the author of this ticket five years ago. Now I think different about this: if type checking at runtime is done here, then there are some hundred other places where it could be done. This would sometimes help programmers new to django and python, ..... but it would slow down the execution time and make the code less clean. I think this ticket can be closed (without a modification to django), but since there where contributions by other people, I will leave this decision to you.

comment:14 Changed 3 years ago by Thomas Güttler

Cc: hv@… removed

comment:15 Changed 3 years ago by Jacob

Resolution: wontfix
Status: newclosed

Yeah, I agree this can be closed wontfix: typechecking isn't particularly Pythonic, and I can think of a few edge-cases where duck-typing a model might be useful.

Note: See TracTickets for help on using tickets.
Back to Top