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

Added support for OrderedDict validation, including as the return type #256

Merged
merged 1 commit into from
Dec 8, 2016
Merged

Added support for OrderedDict validation, including as the return type #256

merged 1 commit into from
Dec 8, 2016

Conversation

SimplyKnownAsG
Copy link
Contributor

This addresses #254, which I created earlier. The fix is pretty simple, sorry for the stupid formatting stuff, I can get rid of those changes as you desire.

One thing I thought was going to be an isuses was ensuring that data is a dict; however, that is already done in the validate_dict closure (right?).

... this is basically the same as a request I had before, except I added a conditional for Python2.6

@@ -1,6 +1,7 @@
import copy
import collections
from nose.tools import assert_equal, assert_raises, assert_true
import numpy
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't add numpy as a dependency, even for tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair point, I'll remove it



def test_ordered_dict():
if not hasattr(collections, 'OrderedDict'):
Copy link
Owner

Choose a reason for hiding this comment

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

I'm fine with this (to answer your earlier question).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great

@coveralls
Copy link

coveralls commented Dec 7, 2016

Coverage Status

Coverage remained the same at 95.968% when pulling 1ab9238 on SimplyKnownAsG:master into f1b5aa8 on alecthomas:master.

@coveralls
Copy link

coveralls commented Dec 7, 2016

Coverage Status

Coverage remained the same at 95.968% when pulling 64c3b07 on SimplyKnownAsG:master into f1b5aa8 on alecthomas:master.

@SimplyKnownAsG
Copy link
Contributor Author

I think I've addressed your comments, but I'm not sure if I need to do anything else here.

Copy link
Collaborator

@tusharmakkar08 tusharmakkar08 left a comment

Choose a reason for hiding this comment

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

The change looks fine. Please squash the commits.

@SimplyKnownAsG
Copy link
Contributor Author

ok, squashed

@coveralls
Copy link

coveralls commented Dec 7, 2016

Coverage Status

Coverage remained the same at 95.968% when pulling 8104b6a on SimplyKnownAsG:master into f1b5aa8 on alecthomas:master.

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