Skip to content

Conversation

@lifus
Copy link

@lifus lifus commented Jan 27, 2016

Review on Reviewable

@lifus lifus force-pushed the get-rid-of-unused-code branch 2 times, most recently from 9a3dd2d to 7d2839a Compare January 28, 2016 07:43

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

itervalues isn't python 3 compatible.

Edit: lots of non python 3 compatible constructs are used. No clue why this doesn't break the tests.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, 2to3 takes care of that:

extra_opts['use_2to3'] = True

You might verify that it gets translated to iterable = iter(items.values()) as mentioned in https://www.python.org/dev/peps/pep-0469/

@lifus lifus force-pushed the get-rid-of-unused-code branch from 7d2839a to cb59356 Compare January 30, 2016 20:26
lifus added 5 commits January 30, 2016 23:31
BaseDict interface is more like dict interface now
It's useless anyway, there are neither instantiations of QuerySetNoDeRef nor calls of __derefence. No client could possibly rely on this class
@lifus lifus force-pushed the get-rid-of-unused-code branch from cb59356 to 989f79d Compare January 30, 2016 20:32
@lifus
Copy link
Author

lifus commented Mar 4, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions.


mongoengine/dereference.py, line 93 [r1] (raw file):
Alright, should I replace itervalues with values now or there going to be a separate task for this? There are quite a few usages of iteritems across the repo at the moment


mongoengine/queryset/queryset.py, line 159 [r2] (raw file):
I would argue that QuerySetNoDeRef is useless since ba48dfb#diff-8e0357331aaa80635a17e55b89ff235f, but I agree that it would be better to raise deprecation warning for a while


Comments from the review on Reviewable.io

@wojcikstefan
Copy link
Member

I believe most of this has been addressed by #1428 already. Thanks anyway!

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