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

A few behavior changes #450

Closed
wants to merge 6 commits into from
Closed

A few behavior changes #450

wants to merge 6 commits into from

Conversation

cag
Copy link

@cag cag commented Sep 23, 2016

I apologize for the title, but I realize this work is actually about multiple things.

  1. to_native and to_primitive of StringType should behave the same. When I serialize objects with a string field through json.dumps, I don't want to have special logic handling to_primitive when the field passed to the model is not a string. Yes, the database should be migrated, but oftentimes it is nice to be able to cast existing objects into strings, and I do expect to_primitive to give me back a str on StringTypes by default.
  2. Interoperability: my local python distro makes .eggs and does not ship with mapping_tests.py
  3. There is an undocumented (AFAIK) assumption by Python in which exceptions are expected to be hashable in order to be added to a stack trace seen_excs set. Since FieldErrors override __eq__, Python states that they are subsequently not hashable unless they also implement __hash__, so I have done so.

A side effect of (1) is that StringTypes also must handle None for to_primitive, as serializing objects with ListTypes and DictTypes containing StringTypes throw ConversionErrors when they contain None.

@cag
Copy link
Author

cag commented Sep 23, 2016

I still don't have mapping_tests.py, but at least this checks out according to Travis...

@lkraider
Copy link
Contributor

The assumption is that to_primitive will only operate on to_native converted data. Any other use is not supported - ie: if you are calling to_primitive outside of Model.serialize you need to take care of the data state yourself.

The problem with your __hash__ implementation is that the FieldError.messages attribute is mutable, that means its hash can change during its lifetime. A correct fix would be to make it immutable.

@lkraider
Copy link
Contributor

Regarding point 3, see also discussion in #452 .

@lkraider
Copy link
Contributor

Work is being done to add immutability to the exceptions, but I am rejecting this PR as it adds incorrect handling for the hashing.

Also, the to_primitive == to_native use case added here is not supported.

Please see PR #477 if you want to help with the immutability discussion.

@lkraider lkraider closed this Mar 22, 2017
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.

None yet

2 participants