Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#22819 closed Cleanup/optimization (fixed)

Rename attribute `output_type` on the Query Expression API

Reported by: jorgecarleitao Owned by: Tim Graham
Component: Database layer (models, ORM) Version: 1.7-beta-2
Severity: Release blocker Keywords:
Cc: jorgecarleitao Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


Since the output_type used in the Query Expression API is becoming part of the Public API in the 1.7,
I would like to propose to rename it before 1.7 is out. Here is why:

output_type is a property of classes that follow the Query Expression API and is defined on any object that follows the Query Expression API as

def output_type(self):
    return IntegerField()  # example, it only requires to be a subclass of `Field`


print(type(IntegerField()))  # <class '__main__.IntegerField'>
print(type(IntegerField))    # <class 'type'>

IMHO, output_type can be misleadingly interpreted as being a type rather than an instance of (a subclass of) a Field.
I imagine it would be very easy to someone write

def output_type(self):
    return CustomField

instead of

def output_type(self):
    return CustomField()

Given that some types that comply with the Query Expression API already have the attribute source (Aggregate, Col, and GeoAggregate), one possibility would be use just output.

Change History (10)

comment:1 Changed 2 years ago by jorgecarleitao

Component: UncategorizedDatabase layer (models, ORM)
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 2 years ago by Tim Graham

Severity: NormalRelease blocker

Bumping to release blocker so we make a decision before 1.7 is released.

comment:3 Changed 2 years ago by Andrew Godwin

Could we not enforce a check that it is a field instance and not a class?

That said, if the impact of renaming it is minimal, let's do it now.

comment:4 Changed 2 years ago by jarshwah

If it were to be renamed, I vote for "output_field", which gives a little more information about what it actually is - an instance of a model_field. Documentation can describe what the current name is, but no such documentation exists (yet) that is clear.

comment:5 Changed 2 years ago by Anssi Kääriäinen

The current name is something I just picked while hacking - there wasn't that much though put into it. The reasoning is something along lines "what data type the DB produces for the expression", hence output_type. Fields happen to represent also database data types, so that is why field instances are used.

If some better name can be found I don't have any objections against changing the name.

comment:6 Changed 2 years ago by Simon Charette

Triage Stage: UnreviewedAccepted

Now we just have to agree on a name. @jorgecarleitao what do you think of output_field?

comment:7 Changed 2 years ago by jorgecarleitao

I agree output_field, even more than output :)

comment:8 Changed 2 years ago by Tim Graham

Owner: changed from nobody to Tim Graham
Status: newassigned

comment:9 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 95cc0e15b45dd0986bdfd9fa3fedee4550c744c8:

Fixed #22819 -- Renamed output_type -> output_field in query expression API.

Thanks jorgecarleitao for the suggestion.

comment:10 Changed 2 years ago by Tim Graham <timograham@…>

In aa10f57d9460f121dea2ec2635e478ed02cc18b5:

[1.7.x] Fixed #22819 -- Renamed output_type -> output_field in query expression API.

Thanks jorgecarleitao for the suggestion.

Backport of 95cc0e15b4 from master

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