-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
[5.6] add withSum to framework #25290
Conversation
add withSum to framework . in the past people try to add withMax withMin withAvg withSum and ... together and implements withCount i think its should be separte because in withSum withAvg and... we should determine what column but in withCount we dont so i add withSum if you think its ok( i test it ) add withAvg and .... . im not pro to do that soory for my bad english
Missing tests, but it's indeed one of the features I miss the most in the framework. |
mm simple test |
Not a chance it will be merged without tests. |
@decadence Agreed, but if there's a positive feedback, I can pick this up, write the tests and re-propose if the author can't write it. @spyp I was talking about automation tests. Inside the framework there's a folder tests containing automation tests for every feature the framework offers. A feature like this requires test-code for quality purposes and to avoid other people from proposing changes that break it's functionality. |
@deleugpn |
@deleugpn as far as I remember there was positive feedback on these PRs but they all missed proper tests which satisfy @taylorotwell. Anyway this is needed and would be cool if you can write them. |
i will added and update here :) |
Hey spyp, it's better if you add integration tests (tests/integration) instead of unit tests for this kind of feature. |
ok i'll add real database too. |
I don't see a big point in adding functions like getRelationshipSumQuery. It feels like it should be generalized to handle all aggregates. Not just sum. |
you are right. i will try to add other aggregates too . |
check it out. in the past people try to add withSum withAvg and ...
and they try to extends the withCount method but i think this methods should be separate because in these methods we play with specific column but in withCount * is enough for that
i test it if you see this is useful add withAvg and etc because im not pro to do that. withSum is useful in
analyze funds and ...