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

[5.1] Fixed the aggregate method #14781

Merged
merged 2 commits into from
Aug 12, 2016
Merged

[5.1] Fixed the aggregate method #14781

merged 2 commits into from
Aug 12, 2016

Conversation

GrahamCampbell
Copy link
Member

It is marked as only returning integers and floats, but it also can return null and string. That's not the correct behaviour.

@ganxiangdong
Copy link

May be you should use “===” replace "!=="

@taylorotwell taylorotwell merged commit 49c6a83 into 5.1 Aug 12, 2016
@GrahamCampbell GrahamCampbell deleted the aggregate branch August 12, 2016 14:39
@JoostK
Copy link
Contributor

JoostK commented Aug 12, 2016

This should be reverted. Some aggregate functions can deal with strings and any existing application using this functionality is now broken.

Furthermore, as @ganxiangdong mentioned this PR is bugged.

@GrahamCampbell you are always very strict about adding tests and preventing behaviour changes in patch releases—which is good, don't get me wrong—yet completely fail to go by your own rules here. I don't understand.

@GrahamCampbell
Copy link
Member Author

Why is the return annotation saying you cannot return anything but an int or a float then?

@GrahamCampbell
Copy link
Member Author

This was an intentional behaviour change to correct it to match the documented return type.

@JoostK
Copy link
Contributor

JoostK commented Aug 12, 2016

@GrahamCampbell I don't know why the annotation is like that, but why change the implementation in favour of changing the annotated return value?

@GrahamCampbell
Copy link
Member Author

Ok, i'll fix this.

@GrahamCampbell
Copy link
Member Author

Please see #14793.

@GrahamCampbell
Copy link
Member Author

Basically reverted this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants