Opened 16 years ago

Closed 9 years ago

#9104 closed Cleanup/optimization (fixed)

FieldDoesNotExist is defined in "confusing" place.

Reported by: Marc Fargas Owned by: nobody
Component: Core (Other) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Django has "django.core.exceptions" package/module where most exceptions are defined, including DB related ones (i.e: ObjectDoesNotExist). Lame users like me would expect FieldDoesNotExist to be defined on the same module but it's in django.db.fields

Maybe we could move the exception?

Change History (12)

comment:1 by Marc Fargas, 16 years ago

Triage Stage: UnreviewedDesign decision needed

comment:2 by Malcolm Tredinnick, 16 years ago

We can't move it (backwards compatibility is important), but we could add an alias in the other place, maybe.

However, the reason for its current location is that this is not an exception that correct user code will ever see. You will see something like ObjectDoesExist because that can happen naturally. But your code should never be catching FieldDoesNotExist, since that would just mean you had made an error in your code 99% of the time (there are some introspection cases where it might not. I guess that is why the admin interface is using it). So declaring the exception where it is actually used and then only using it internally isn't a bad thing.

What's the use-case where you actually need to ever import this exception?

comment:3 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:4 by Luke Plant, 13 years ago

Severity: Normal
Type: Cleanup/optimization

comment:5 by Aymeric Augustin, 12 years ago

Easy pickings: unset
Resolution: needsinfo
Status: newclosed
UI/UX: unset

Malcolm explained why the code is written in this way, and his question stays unanswered after years.

comment:6 by nickname123, 10 years ago

Just came across this looking for the exception with Google search.

My use case is that I have a model form with extra fields on the form than what the model defines. I use these extra fields to auto populate some of the model fields.

I am iterating through the form fields to add HTML5 attrs during init. I need to catch FieldDoesNotExist when I try to check if the model field has blank=True to determine if the formfield is required or not.

from django.db.models.fields import FieldDoesNotExist

def __init__(self, *args, **kwargs):
        super(FooModel, self).__init__(*args, **kwargs)
        for key in self.fields:
            try:
                mfield = self.instance._meta.get_field_by_name(key)[0]
            except FieldDoesNotExist:
                pass
            else: 
                self.fields[key].widget.attrs["required"] = not mfield.blank

This isn't too important, so I am just leaving the comment in case Google brings me back here again before I grep.

Version 1, edited 10 years ago by nickname123 (previous) (next) (diff)

comment:7 by wraus, 10 years ago

Resolution: needsinfo
Status: closednew

I'd like to reopen this just to hopefully have this oddity resolved.

One of the comments was talking about use cases, and I wanted to give the use case that I'm using for it to hopefully sway opinion a bit on making this simple change.

I'm working on a template tag that will allow me to iterate over a list of a model's fields / attributes and output it as a table. I want to grab the verbose_name of the field, given the field's name, and to do so, I get obj._meta.get_field(field_name). However, I also want the option to reference class attributes and functions, and obviously if I reference a function or attribute, there is no matching field. Thus, I need to call get_field, and catch FieldDoesNotExist to handle it as a function / attribute and get a separate label.

There doesn't seem to be a more "obvious" way to do this, and I ran into the issue of trying to find this oddly placed exception. I understand that moving the exception would be a serious change, but hopefully making it importable via django.core.exceptions will resolve this inconvenience.

comment:8 by Tom Christie, 9 years ago

I believe that the correct resolution of this should be whatever decision is made in the official 'meta' API update.

Given that we don't yet document get_fields or FieldDoesNotExist anywhere, the new public API is the correct way to resolve this one way or the other.

https://github.com/django/django/pull/3114

comment:9 by Russell Keith-Magee, 9 years ago

This has been resolved as part of the Meta-refactor work, which is part of PR3114 - see commit f96990ff.

comment:10 by Claude Paroz, 9 years ago

Even if it is not documented, searching for FieldDoesNotExist on github for example reveals many usages of this import location in the wild, hence I think we should provide some deprecation path.

It should be possible for example to create a django/db/models/fields/FieldDoesNotExist.py module to trigger the deprecation warning on old import location (don't know if there is a cleaner way to deprecate import locations).

comment:11 by Claude Paroz, 9 years ago

Of course, my "trick" about a FieldDoesNotExist.py file only works if we want to loudly fail with an explicit message. Still looking for a way to raise a warning without ever calling the symbol (might be impossible...).

comment:12 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: newclosed

In 8958170755b37ce346ae5257c1000bd936faa3b0:

Fixed #9104 -- Moved FieldDoesNotExist to core.exceptions

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