Skip to content

Conversation

@ms5
Copy link

@ms5 ms5 commented Mar 8, 2016

fix for issue #904 includes a test with the test case i reported initially.

Review on Reviewable

…error case I reported in the issue report.
@lafrech
Copy link
Member

lafrech commented Mar 8, 2016

+        if PY3:
+            son_gen = son.items()
+        else:
+            son_gen = son.iteritems()

AFAIU, PY3 is dealt with using 2to3 and elsewhere in the code, only PY2's iteritems is used.

I would have done the same here for consistency, but I'm not sure what the best practice is.

There are indeed see a few if PY3 statements in the code, probably where 2to3 couldn't manage.

@ms5
Copy link
Author

ms5 commented Mar 8, 2016

thats correct.iteritems() is enough. I've removed the py3 code. I've also added two more test, that check the actual translation to and from SON.

Copy link
Member

@wojcikstefan wojcikstefan left a comment

Choose a reason for hiding this comment

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

Thanks for this @ms5 and sorry the conversation stalled for so long. Are you still interested in merging this PR? If so, I'll be happy to work with you on it!

class DbFieldParameterTest(unittest.TestCase):

def setUp(self):
# testcase from https://github.com/MongoEngine/mongoengine/issues/904
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to add a docstring to the class above describing exactly what this set of tests ensures. Otherwise, we're forcing every dev to look up a (somewhat poorly worded) GitHub ticket in order to learn what this is about.

try:
a.validate()
except ValidationError as e:
self.fail("ValidationError raised: %s" % e.message)
Copy link
Member

Choose a reason for hiding this comment

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

No need to have try-except here. Can be replaced with a.validate() # the test passes if this doesn't raise a ValidationError.

@wojcikstefan
Copy link
Member

Fixed in #1501

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