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

Handle aggregate types a very tiny bit better #569

Merged
merged 1 commit into from
Jul 23, 2023
Merged

Conversation

gwynne
Copy link
Member

@gwynne gwynne commented Jun 15, 2023

This just adds explicit forwarding of the generic Result parameter of the two QueryBuilder.aggregate() variants which weren't doing so already. It was already being implicitly forwarded due to type inference, but better to do so explicitly.

Also adds notes to the sum() and average() sets of methods warning that the Field.Value return types are not always correct (usually only causes problems with Postgres) and giving the workaround. This is related to #379 and vapor/fluent-postgres-driver#166 but does NOT fix those issues; it only clarifies one of the ways to get around the problem.

…ate() methods, notate the return type problem for sum and average aggregation
@gwynne gwynne added bug Something isn't working semver-patch Internal changes only labels Jun 15, 2023
Copy link

@dannflor dannflor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides thinking the workaround should be broadcast publicly, LGTM

@codecov-commenter
Copy link

codecov-commenter commented Jun 15, 2023

Codecov Report

Merging #569 (8bafe01) into main (2d7dce5) will increase coverage by 0.03%.
The diff coverage is 87.50%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #569      +/-   ##
==========================================
+ Coverage   47.53%   47.56%   +0.03%     
==========================================
  Files         106      106              
  Lines        4706     4709       +3     
==========================================
+ Hits         2237     2240       +3     
  Misses       2469     2469              
Impacted Files Coverage Δ
...uentKit/Query/Builder/QueryBuilder+Aggregate.swift 49.36% <87.50%> (+1.99%) ⬆️

@gwynne gwynne requested a review from dannflor June 15, 2023 22:06
Copy link

@dannflor dannflor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for holding up the PR, probably should've just left a comment.

@gwynne gwynne merged commit cee27b8 into main Jul 23, 2023
@gwynne gwynne deleted the gwynne-patch-1 branch July 23, 2023 09:02
@penny-for-vapor
Copy link

These changes are now available in 1.43.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working semver-patch Internal changes only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants