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.

Sign up for GitHub

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

Jump to bottom

Missing parent aggregation #1614

Closed
SosthenG opened this issue Mar 5, 2019 · 2 comments
Closed

Missing parent aggregation #1614

SosthenG opened this issue Mar 5, 2019 · 2 comments

Comments

@SosthenG
Copy link
Contributor

SosthenG commented Mar 5, 2019 •
edited
Loading

Version 6.6 introduced the ParentAggregation which is missing in Elastica.

I worked on it on my own fork because I needed it.

Before I can open a PR I have one question: is there a good way to handle the class name? "Parent" is a reserved word so I named the class "ParentAggregation" and overrided the _getBaseName method to return "parent" which seems to allow the params conversion to work.
But I'm not sure it was the good way to do it...

Actually it seems that for the GlobalAggregation the solution was to add a special case to the toArray method of the AbstractAggregation, I'm not sure which one is the best.
@ruflin
Copy link
Owner

ruflin commented Mar 6, 2019

It's always a tricky one with the reserved names. Could you open your PR with what you have right now so we can discuss it directly in the code. At least for me, that would make it easier.

@SosthenG
Copy link
Contributor Author

SosthenG commented Mar 6, 2019

Ok, done #1616

@SosthenG SosthenG closed this as completed Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@ruflin @SosthenG