Code

Opened 6 years ago

Closed 3 years ago

#5931 closed Cleanup/optimization (fixed)

__repr__ for db Fields

Reported by: Thomas Güttler <hv@…> Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: hv@…, mathieu.agopian@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX:

Description

Hi,

for debugging it is usefull, if repr(mydbfield) displays the name
of the field. Small patch inclusive test attached.

Attachments (4)

db_models_fields_repr.diff (1.1 KB) - added by Thomas Güttler <hv@…> 6 years ago.
5931.repr_for_model_fields.diff (1.3 KB) - added by emulbreh 4 years ago.
added a test
5931-__repr__-for-db-fields (1.5 KB) - added by magopian 3 years ago.
updated patch that applies to the current trunk
5931-__repr__-for-db-fields.diff (1.7 KB) - added by magopian 3 years ago.
updated patch taking claudep's and julien's comments into account

Download all attachments as: .zip

Change History (16)

Changed 6 years ago by Thomas Güttler <hv@…>

comment:1 Changed 6 years ago by mattmcc

  • Component changed from Uncategorized to Database wrapper
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 5 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Design decision needed to Accepted

Seems reasonable. However, the test doesn't show that it works as expected. It shows that when you use a field in a completely non-standard fashion (outside of a model), it reports None. That's maybe 0.001% of the normal use-cases. Testing the other 99%+ would be ideal. :-)

Changed 4 years ago by emulbreh

added a test

comment:3 Changed 4 years ago by anonymous

  • Patch needs improvement unset

comment:4 Changed 3 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:5 Changed 3 years ago by julien

  • Easy pickings set
  • Patch needs improvement set

This looks great but it'd be even better if the tests also covered the 0.001% of use cases (i.e. when the field does not have a name) :-)

comment:6 Changed 3 years ago by magopian

  • Cc mathieu.agopian@… added
  • Patch needs improvement unset

Here is an updated patch that applies on the current trunk.

I believe there's no way a django.db.models.fields.Field won't have a name anymore though.

Changed 3 years ago by magopian

updated patch that applies to the current trunk

comment:7 follow-up: Changed 3 years ago by claudep

  • Patch needs improvement set

I think that Julien's comment was referring to a test like:

from django.db.model.fields import CharField
my_field = CharField()
repr(my_field)

that should reveal the None value in place of the name.

comment:8 in reply to: ↑ 7 Changed 3 years ago by julien

Replying to claudep:

I think that Julien's comment was referring to a test like:

from django.db.model.fields import CharField
my_field = CharField()
repr(my_field)

that should reveal the None value in place of the name.

That's correct. There's nothing preventing you from using a Field on its own, hence the "getattr(self, 'name', None))" in the patch ;)

comment:9 Changed 3 years ago by julien

That said, I also think that <django.db.models.fields.CharField None> would be rather confusing. If the field doesn't have a name, then simply displaying <django.db.models.fields.CharField> would be preferable in my opinion.

Changed 3 years ago by magopian

updated patch taking claudep's and julien's comments into account

comment:10 Changed 3 years ago by magopian

  • Patch needs improvement unset

I understand now ;) thanks for the feedbacks and comments!

I've attached an updated patch, with the additional test and the specific case for the Field instantiated without a name.

Please let me know if this needs further improvements!

comment:11 Changed 3 years ago by aaugustin

  • Triage Stage changed from Accepted to Ready for checkin

comment:12 Changed 3 years ago by jezdez

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

In [16145]:

Fixed #5931 -- Added repr to db fields. Thanks, Thomas Güttler, emulbreh and magopian.

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.