Normalising tuples support #271 #301

Merged
merged 1 commit into from Mar 21, 2017

Conversation

Projects
None yet
3 participants
@elecay
Contributor

elecay commented Mar 17, 2017

No description provided.

@funkyfuture

though i'm pretty sure that it is covered, i would be reassured if you tested that the items' order is maintained in the normalized result of a sequence.

cerberus/validator.py
@@ -666,8 +666,11 @@ def __normalize_sequence(self, field, mapping, schema):
document_crumb=field, schema_crumb=(field, 'schema'),
schema=schema)
result = validator.normalized(document, always_return_document=True)
- for i in result:
- mapping[field][i] = result[i]
+ if len(result.values()) > 0:

This comment has been minimized.

@funkyfuture

funkyfuture Mar 18, 2017

Member

is that test necessary? if results: would be more idiomatic.

@funkyfuture

funkyfuture Mar 18, 2017

Member

is that test necessary? if results: would be more idiomatic.

cerberus/validator.py
- for i in result:
- mapping[field][i] = result[i]
+ if len(result.values()) > 0:
+ if type(mapping[field]) is tuple:

This comment has been minimized.

@funkyfuture

funkyfuture Mar 18, 2017

Member

that wouldn't work with subclasses of tuple, therefore the isinstance function is common to identify types.
nonetheless, it should be enough to create a new object of a particular type: mapping[field] = type(mapping[field])(result.values)

@funkyfuture

funkyfuture Mar 18, 2017

Member

that wouldn't work with subclasses of tuple, therefore the isinstance function is common to identify types.
nonetheless, it should be enough to create a new object of a particular type: mapping[field] = type(mapping[field])(result.values)

This comment has been minimized.

@elecay

elecay Mar 18, 2017

Contributor

mind-blowing for me

@elecay

elecay Mar 18, 2017

Contributor

mind-blowing for me

This comment has been minimized.

@funkyfuture

funkyfuture Mar 18, 2017

Member

don't get addicted or you'll find yourself using Python for the laundry. ;-)

@funkyfuture

funkyfuture Mar 18, 2017

Member

don't get addicted or you'll find yourself using Python for the laundry. ;-)

@funkyfuture

This comment has been minimized.

Show comment
Hide comment
@funkyfuture

funkyfuture Mar 18, 2017

Member

oh, and for fixing pull requests i recommend to squash your commits (or rather fix) and then force-push your branch.

Member

funkyfuture commented Mar 18, 2017

oh, and for fixing pull requests i recommend to squash your commits (or rather fix) and then force-push your branch.

@elecay

This comment has been minimized.

Show comment
Hide comment
@elecay

elecay Mar 18, 2017

Contributor

Awesome comments. Will fix this.

Contributor

elecay commented Mar 18, 2017

Awesome comments. Will fix this.

elecay added a commit to elecay/cerberus that referenced this pull request Mar 18, 2017

Some idiomatic fixing
Add corrections according @funkyfuture recomendations
(pyeve#301 (review))
cerberus/validator.py
@@ -666,8 +666,8 @@ def __normalize_sequence(self, field, mapping, schema):
document_crumb=field, schema_crumb=(field, 'schema'),
schema=schema)
result = validator.normalized(document, always_return_document=True)
- for i in result:
- mapping[field][i] = result[i]
+ if result:

This comment has been minimized.

@funkyfuture

funkyfuture Mar 18, 2017

Member

with my previous comment i meant two things, sorry for being unclear:

  • this expression is semantically identical to the previous version
  • the expression shouldn't be needed at all, but i might be wrong with that assumption
@funkyfuture

funkyfuture Mar 18, 2017

Member

with my previous comment i meant two things, sorry for being unclear:

  • this expression is semantically identical to the previous version
  • the expression shouldn't be needed at all, but i might be wrong with that assumption

This comment has been minimized.

@elecay

elecay Mar 18, 2017

Contributor

You are totally right. Sorry about that, and thank you for your guide and your time. 😄

@elecay

elecay Mar 18, 2017

Contributor

You are totally right. Sorry about that, and thank you for your guide and your time. 😄

@nicolaiarocci nicolaiarocci added this to the 1.2 milestone Mar 20, 2017

@funkyfuture

This comment has been minimized.

Show comment
Hide comment
@funkyfuture

funkyfuture Mar 20, 2017

Member

i tested it against a more nasty sequence and still found it to work. seems that value views are sorted in all Python version, which i didn't expect.

we could also use that type instantiation for the normalized mappings to maintain the input's type.

Member

funkyfuture commented Mar 20, 2017

i tested it against a more nasty sequence and still found it to work. seems that value views are sorted in all Python version, which i didn't expect.

we could also use that type instantiation for the normalized mappings to maintain the input's type.

@elecay

This comment has been minimized.

Show comment
Hide comment
@elecay

elecay Mar 20, 2017

Contributor

Great. Should I do something else or can we consider this issue closed?

Contributor

elecay commented Mar 20, 2017

Great. Should I do something else or can we consider this issue closed?

@funkyfuture

This comment has been minimized.

Show comment
Hide comment
@funkyfuture

funkyfuture Mar 20, 2017

Member

i'd consider it solved. i'll make a follow-up for the mappings later.

Member

funkyfuture commented Mar 20, 2017

i'd consider it solved. i'll make a follow-up for the mappings later.

@nicolaiarocci nicolaiarocci merged commit d3ed9dc into pyeve:master Mar 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

nicolaiarocci added a commit that referenced this pull request Mar 21, 2017

@nicolaiarocci

This comment has been minimized.

Show comment
Hide comment
@nicolaiarocci

nicolaiarocci Mar 21, 2017

Member

Thanks!

Member

nicolaiarocci commented Mar 21, 2017

Thanks!

@nicolaiarocci

This comment has been minimized.

Show comment
Hide comment
Member

nicolaiarocci commented Mar 21, 2017

Closes #271

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment