Skip to content

Conversation

@wojcikstefan
Copy link
Member

Fixes #1415

@lafrech @zeezdev take a look please

@wojcikstefan
Copy link
Member Author

@yograterol just curious, is the "In Progress" label assignment an automated hook of some sort? Does anybody find it useful (cc @lafrech)?

@lafrech lafrech merged commit 7e6287b into master Dec 2, 2016
@lafrech
Copy link
Member

lafrech commented Dec 2, 2016

I have no idea what this InProgress tag is for and how it is generated.

@zeezdev
Copy link
Contributor

zeezdev commented Dec 3, 2016

It works, but to sum it is necessary to specify the names of the models instead of the field names in the database that is not compatible with old behavior of aggregate_sum.

@wojcikstefan
Copy link
Member Author

wojcikstefan commented Dec 4, 2016

Hm, using db fields directly seems like a invalid behavior. That being said, I understand aggregate_sum worked this way and that we should have added a warning in the changelog stating that the new sum doesn't follow the same logic. As far as I remember though, current behavior is consistent with the way the old sum/average worked.

@lafrech
Copy link
Member

lafrech commented Dec 4, 2016

As far as I remember though, current behavior is consistent with the way the old sum/average worked.

Then you couldn't do much better. It would have been complicated to both fit old sum/avg behaviour and keep compatibility with aggregate_sum/avg, if both behaved differently.

The aggregate_ versions were not advertised in the docs, so from the docs perspective, nothing is broken. Yet, you may want to add a note to the docs:

https://github.com/MongoEngine/mongoengine/blob/master/docs/guide/querying.rst#further-aggregation

to specify that it works with the model names, and to warn about the change in latest MongoEngine:

  • now using aggreg framework
  • removal of aggregation_*

for anyone being affected by the change.

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.

4 participants