-
-
Notifications
You must be signed in to change notification settings - Fork 744
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
Fix aggregation query parameters to correctly replace keys in list #1035
Conversation
eve/methods/get.py
Outdated
parse_aggregation_stage(st_value, key, value) | ||
if key == st_value: | ||
d[st_key] = value | ||
if isinstance(d, list): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
elif
eve/methods/get.py
Outdated
d[st_key] = value | ||
if isinstance(d, dict): | ||
for st_key, st_value in d.items(): | ||
if isinstance(st_value, dict) or isinstance(st_value, list): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you create a method in order to avoid duplication?
eve/methods/get.py
Outdated
if isinstance(st_value, dict) or isinstance(st_value, list): | ||
parse_aggregation_stage(st_value, key, value) | ||
if key == st_value: | ||
d[st_idx] = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us see what @nicolaiarocci says, but maybe it is worth to report it in the documentation related to the aggregation endpoint
Hi, Can you please squash the commit into one? Btw, the continuos integration is failing! :) Regards, |
Hello, this is very nice. I am willing to merge. Would you also provide test coverage? |
Do you need some help with this issue? I've already experimented with aggregation parameters and also with adding pre_aggregation and on_fetched_aggregation hooks. Currently also without test coverage but I can write them. |
@idserge7: without test coverage I do not think @nicolaiarocci is willing to merge! But please, do your PR! :) |
I'll cover it with tests first :) |
Is anyone on test coverage for this? |
Nevermind we have a similar PR with proper testing. Will go for that one instead: #1058 |
This commit resolves #1025.