Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #29317 -- Clarified filter argument in PostgreSQL aggregate functions. #10530

Merged
merged 1 commit into from Nov 27, 2018

Conversation

shalgrim
Copy link
Contributor

First time contributor at DjangoCon US 2018 making a documentation change per Issue #29317.

The discussion in that ticket thread indicated the best fix to the confusion in the existing documentation was to add this note with this link.

@timgraham timgraham changed the title adds note to filtering docs per Issue #29317 Fixed #29317 -- Clarified filter argument in PostgreSQL aggregate functions. Oct 19, 2018
@timgraham
Copy link
Member

I'm not sure -- it feels odd to give a special call out like that. I'm thinking it may make more sense not to document filter=None in every subclass and instead leave it at **extra, similar to model fields where we don't document all the common options for every field. \cc @orf

@VishvajitP VishvajitP mentioned this pull request Oct 23, 2018
@timgraham timgraham force-pushed the ticket_29317 branch 2 times, most recently from 9cea20e to 23a5ce2 Compare November 27, 2018 15:57
@timgraham
Copy link
Member

@charettes, what do you think of this approach? (As opposed to documenting distinct for every function in #9174.)

@charettes
Copy link
Member

@timgraham I think I'd make sense. Something I noticed while working on #9174 is that we already document it for all aggregates in the Aggregate documentation.

The ``filter`` argument takes a :class:`Q object <django.db.models.Q>` that's
used to filter the rows that are aggregated. See :ref:`conditional-aggregation`
and :ref:`filtering-on-annotations` for example usage.

Maybe we should just like there from the contrib.postgres.aggregate documentation? filter happens to be implemented differently on PostgreSQL but it's support on all backends so I'm not sure why it should be special cased in contrib.postgres docs.

@timgraham
Copy link
Member

Yea, okay. I guess removing it from the signature could be confusing.

@timgraham timgraham merged commit 926fa71 into django:master Nov 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants