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

Feature/aggregate multi params #24

Merged
merged 4 commits into from
Dec 15, 2016

Conversation

szykin
Copy link
Contributor

@szykin szykin commented Dec 14, 2016

Aggregate now accepts multiple positional and keyword arguments, just like in Django.
Second commit is a quick fix so distinction of positional arguments will happen before loop.
If you think some of the tests I added here are overkill, feel free to remove them.

@codecov-io
Copy link

codecov-io commented Dec 14, 2016

Current coverage is 100% (diff: 100%)

Merging #24 into master will not change coverage

@@           master   #24   diff @@
===================================
  Files           6     6          
  Lines         346   350     +4   
  Methods         0     0          
  Messages        0     0          
  Branches        0     0          
===================================
+ Hits          346   350     +4   
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last update cdd9b04...fb313a8

@stphivos
Copy link
Owner

Thanks! Yeah all the tests seem useful here and serve a different purpose, except maybe the first (test_query_aggregate_with_none_only_field_values_performs_correct_aggregation)
seems like a subset of the third (test_query_aggregate_multiple_params_with_none_only_field_values_aggregation_with_none)? But even so we can clean it up in the future if we determine it's not needed!

@stphivos stphivos merged commit 65658d3 into stphivos:master Dec 15, 2016
@szykin szykin deleted the feature/aggregate_multi_params branch December 15, 2016 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants