Code

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#13921 closed (wontfix)

Inconsistent get_internal_type on official models' fields

Reported by: mitar Owned by: nobody
Component: Database layer (models, ORM) Version: 1.2
Severity: Keywords:
Cc: mmitar@… Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I think there is a small inconsistency on when does get_internal_type return base type and when it defines as a new type in official models' fields.

I wanted to make a dynamic list of searchable fields (search_fields) in admin by inspecting all defined fields in a model and adding all those which have get_internal_type defined as CharField or TextField (and ignoring those which define choices, but this is not important here). This works great on EmailField and URLField among official models' fields. But there is a strange situation with SlugField and CommaSeparatedIntegerField. The first defines new type (named SlugField) which is OK by me if we say that it changes semantics somewhat. But then why CommaSeparatedIntegerField does not define new type but leave it as CharField where this field is definitely something special.

I would suggest SlugField returns CharField and that CommaSeparatedIntegerField returns CommaSeparatedIntegerField. Or to do at least just the later.

Attachments (0)

Change History (2)

comment:1 Changed 3 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to wontfix
  • Status changed from new to closed

I'm going to mark this wontfix on the grounds that it is a bunch of code shuffling that doesn't yield any particular benefit -- at least, the ticket doesn't suggest any particular benefit.

Yes, there is a minor inconsistency. SlugField should probably just reference the CharField type. But maintaining the inconsistency takes no effort, and removing it will require a bunch of effort implementing a deprecation timeline. Absent of a good reason to make this change, wontfix seems like the best approach to me.

comment:2 Changed 3 years ago by mitar

As I explained, it is useful to be able to automatically define fields over which to search over. get_internal_type is probably meant just for such cases where you need a way to inspect things dynamically. Having inconsistencies there defeats its purpose.

I do not believe this is much of code shuffling, just changing few lines (return values of get_internal_type). And as things are currently I do not believe there is a need for a deprecation timeline, as this return values are not quite predictable so any code using get_internal_type have to already be robust to handle different cases.

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.