Code

Opened 6 years ago

Closed 13 months ago

#6702 closed Cleanup/optimization (wontfix)

ModelForm: assert given instance is instance of model

Reported by: guettli Owned by:
Component: Forms Version: master
Severity: Normal Keywords:
Cc: selwin, 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 guettli 6 years ago.

Download all attachments as: .zip

Change History (16)

Changed 6 years ago by guettli

comment:1 Changed 6 years ago by guettli

  • 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 6 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 6 years ago by mtredinnick

  • Triage Stage changed from Unreviewed to Accepted

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 5 years ago by guettli

  • Needs tests set

comment:5 Changed 4 years ago by SmileyChris

  • Owner changed from nobody to SmileyChris
  • Status changed from new to assigned

comment:6 Changed 4 years ago by SmileyChris

  • Owner SmileyChris deleted
  • Status changed from assigned to new

comment:7 Changed 3 years ago by julien

  • Type set to Cleanup/optimization

comment:8 Changed 3 years ago by julien

  • Severity set to Normal

comment:9 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:10 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:11 Changed 20 months ago by selwin

  • Cc selwin 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 13 months ago by bmispelon

  • 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 13 months ago by guettli

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 13 months ago by guettli

  • Cc hv@… removed

comment:15 Changed 13 months ago by jacob

  • Resolution set to wontfix
  • Status changed from new to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.