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

adding group by #403

Merged
merged 6 commits into from Mar 25, 2018

Conversation

3 participants
@rafiki270
Copy link
Contributor

rafiki270 commented Mar 10, 2018

@tanner0101 I am not sure how to add the custom group by (to allow for CONCAT and other special computed stuff like YEAR etc ...) ... would it be fine to just allow string input?

Also, how do you handle Package file in PR's when the target hasn't been released yet?

@rafiki270

This comment has been minimized.

Copy link
Contributor Author

rafiki270 commented Mar 12, 2018

1: can you make the PR into the nio branch? that will be merged in a few days
so let's avoid conflicts and just put it directly to there
2: we need to hide the public enum QueryGroupBy
make this as a struct with a static func field instead
you can use an internal enum as storage
but if we ever want to add a new type there, we need to make sure it's not an enum or we are stuck with it
https://github.com/vapor/fluent/blob/nio/Sources/Fluent/Query/Filter/QueryFilter.swift#L26-L57
take a look at how it's achieved there ^
and here: https://github.com/vapor/fluent/blob/nio/Sources/Fluent/Query/Filter/QueryFilter.swift#L61
that's how you make something that feels like an enum, but is a struct

@tanner0101 tanner0101 added this to the 3.0.0-rc.2 milestone Mar 12, 2018

@tanner0101 tanner0101 self-assigned this Mar 12, 2018

@calebkleveter

This comment has been minimized.

Copy link
Member

calebkleveter commented Mar 14, 2018

Does it follow the Swift API guidelines better to call it group(by:) or groupBy(_:)? I honestly like the way the first one looks better.

@rafiki270 rafiki270 force-pushed the rafiki270:group-by branch from bdd46a7 to a8d78c4 Mar 14, 2018

@rafiki270

This comment has been minimized.

Copy link
Contributor Author

rafiki270 commented Mar 14, 2018

doing a NIO PR instead

@rafiki270 rafiki270 closed this Mar 14, 2018

@rafiki270 rafiki270 reopened this Mar 14, 2018

@rafiki270 rafiki270 changed the base branch from master to nio Mar 14, 2018

@rafiki270

This comment has been minimized.

Copy link
Contributor Author

rafiki270 commented Mar 14, 2018

@tanner0101 @calebkleveter all amended

@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented Mar 22, 2018

@rafiki270 looks like linux-postgresql test is failing. Can you merge the latest copy of master into your branch, that will probably fix it.

@tanner0101 tanner0101 removed this from the 3.0.0-rc.2 milestone Mar 22, 2018

@rafiki270 rafiki270 force-pushed the rafiki270:group-by branch from 7d7b6c4 to b1823fa Mar 22, 2018

@rafiki270

This comment has been minimized.

Copy link
Contributor Author

rafiki270 commented Mar 22, 2018

@rafiki270 rafiki270 force-pushed the rafiki270:group-by branch from b1823fa to 921c109 Mar 23, 2018

@tanner0101 tanner0101 changed the base branch from nio to master Mar 25, 2018

@tanner0101
Copy link
Member

tanner0101 left a comment

LGTM, just one small fix needed.


/// GROUP BY statement for QueryBuilder
public struct QueryGroupBy {
public enum Storage {

This comment has been minimized.

@tanner0101

tanner0101 Mar 25, 2018

Member

this enum needs to be private, as well as the storage property

@rafiki270

This comment has been minimized.

Copy link
Contributor Author

rafiki270 commented Mar 25, 2018

@tanner0101 tanner0101 added this to the 3.0.0-rc.2.2 milestone Mar 25, 2018

@tanner0101
Copy link
Member

tanner0101 left a comment

woot, ty @rafiki270!

@tanner0101 tanner0101 merged commit ff1d9a9 into vapor:master Mar 25, 2018

4 checks passed

ci/circleci: linux Your tests passed on CircleCI!
Details
ci/circleci: linux-mysql Your tests passed on CircleCI!
Details
ci/circleci: linux-postgresql Your tests passed on CircleCI!
Details
ci/circleci: linux-sqlite Your tests passed on CircleCI!
Details

@rafiki270 rafiki270 deleted the rafiki270:group-by branch Mar 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment