[WIP] Allow modification of data on [de]serialization #34
Conversation
|
This is messy but I like to share sooner than later and I have someone specific to talk to about it. I'll note when I have some level of confidence that I like this. |
|
Howdy! Im alive! This is an interesting idea. Can you take a stab at a paragraph or two in the readme explaining the feature? |
|
Sure thing, I added a list to the initial comment. If you have any design or API concerns along the way do bring them up. I'm not strongly settled on anything at this point. |
tests/test_serialization.py
Outdated
|
|
||
| top_class_str_example = TopClass(polyfield=u'abc') | ||
| top_class_str_example_dumped = top_schema.dump(top_class_str_example) | ||
| print(top_class_str_example_dumped) |
There was a problem hiding this comment.
rather than printing I think you should validate the json that comes out with an assertion
There was a problem hiding this comment.
This is just exploratory code and I think it's fully replaced at this point by 'real' tests. It's in the WIP list to be removed.
tests/test_serialization.py
Outdated
| top_class_str_example_loaded = top_schema.load(top_class_str_example_dumped) | ||
| assert top_class_str_example_loaded == top_class_str_example | ||
|
|
||
| print('---') |
There was a problem hiding this comment.
Personally id rather you pull out what you need so you can make these two different tests rather than one test separated by the output
There was a problem hiding this comment.
Like above, to be removed.
|
So this looks pretty reasonable. PIng me when you think you have it at a state where its ready to merge! |
|
@Bachmann1234, it's not ready to merge but I have no particular outstanding issues on my list presently. So, it's at least worth a fairly real review I think. I may start into a PR against desert using this before really wanting to have this merged. It would be a good test of the implementation here and put me in a different seat looking at it. |
|
This is looking pretty good. I think the conflicts are due to the PR dropping 27 so you may want to resolve that before any other tweaking |
|
^^ What im seeing on the merge section (I know you still dont are not ready to merge but just letting you know) |
|
hmm... have you reloaded the page recently?
|
|
Ah, I figured it out. I'm defaulted to rebase and merge. That won't work due to conflcits. But I can squash and merge just fine |
|
Or, just merge? :] But sure, lots of personal preference to be had there. |
#33
WIP for:
[ ] docsPolyFieldBasemethodsExplicitPolyFieldat the top level?implement something for a 'default' sort of class/schema mapping ala{str: fields.String, int: fields.Integer, ...}