Opened 9 years ago

Closed 5 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@…> 9 years ago.
5931.repr_for_model_fields.diff (1.3 KB) - added by Johannes Dollinger 7 years ago.
added a test
5931-__repr__-for-db-fields (1.5 KB) - added by Mathieu Agopian 5 years ago.
updated patch that applies to the current trunk
5931-__repr__-for-db-fields.diff (1.7 KB) - added by Mathieu Agopian 5 years ago.
updated patch taking claudep's and julien's comments into account

Download all attachments as: .zip

Change History (16)

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

Attachment: db_models_fields_repr.diff added

comment:1 Changed 9 years ago by Matt McClanahan

Component: UncategorizedDatabase wrapper
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedDesign decision needed

comment:2 Changed 8 years ago by Malcolm Tredinnick

Patch needs improvement: set
Triage Stage: Design decision neededAccepted

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 7 years ago by Johannes Dollinger

added a test

comment:3 Changed 7 years ago by anonymous

Patch needs improvement: unset

comment:4 Changed 5 years ago by Gabriel Hurley

Severity: Normal
Type: Cleanup/optimization

comment:5 Changed 5 years ago by Julien Phalip

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

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

Attachment: 5931-__repr__-for-db-fields added

updated patch that applies to the current trunk

comment:7 Changed 5 years ago by Claude Paroz

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

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

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

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

comment:10 Changed 5 years ago by Mathieu Agopian

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

Triage Stage: AcceptedReady for checkin

comment:12 Changed 5 years ago by Jannis Leidel

Resolution: fixed
Status: newclosed

In [16145]:

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

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