Opened 11 months ago

Closed 10 months ago

Last modified 10 months ago

#22819 closed Cleanup/optimization (fixed)

Rename attribute `output_type` on the Query Expression API

Reported by: jorgecarleitao Owned by: timo
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

Description

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

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

However,

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

@property
def output_type(self):
    return CustomField

instead of

@property
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 11 months ago by jorgecarleitao

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 10 months ago by timo

  • Severity changed from Normal to Release blocker

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

comment:3 Changed 10 months ago by andrewgodwin

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 10 months ago by smeatonj

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 10 months ago by akaariai

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 10 months ago by charettes

  • Triage Stage changed from Unreviewed to Accepted

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

comment:7 Changed 10 months ago by jorgecarleitao

I agree output_field, even more than output :)

comment:8 Changed 10 months ago by timo

  • Owner changed from nobody to timo
  • Status changed from new to assigned

comment:9 Changed 10 months ago by Tim Graham <timograham@…>

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

In 95cc0e15b45dd0986bdfd9fa3fedee4550c744c8:

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

Thanks jorgecarleitao for the suggestion.

comment:10 Changed 10 months 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