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

Fix schema problems #66

Merged
merged 7 commits into from
Feb 28, 2019
Merged

Fix schema problems #66

merged 7 commits into from
Feb 28, 2019

Conversation

Peilonrayz
Copy link
Contributor

@Peilonrayz Peilonrayz commented Feb 19, 2019

In light of #68 I have updated the code to fix #57, #58, #60, #65, #67 and #68. I have added relevant tests and ensured they pass in Python 3.6 and 3.7. (I've changed the code to allow skipping of the datetime tests.)

From this I have moved all schema code into mm so that the marshmallow code is contained. And this is basically just a re-write of mm. I've updated api to accommodate these changes.

Otherwise only core has been changed. This is because marshmallow now loads schema data into the relevant model. (This can be tested against with code in #58). I'm unsure how you'd want the data to be handled, and so I chose to return early so I don't mangle with core too. please verify this is the intended behavior.

This should keep all the previous logic around options, most are in schema such as specifying a default, and handling Optional. The other special piece of logic is when creating a DC, I kept the field_many option. Otherwise build_type now builds all the types you'd previously specified, and a couple more, in a recursive form, defaulting to fields.Field and a warning.


I would like to request that we change how you handle the dataclasses_json.mm_field so that if the value is None we generate one by default. This isn't something you did previously, and so I didn't change this.

@Peilonrayz Peilonrayz changed the title Fix #60 Update _make_default_field to work with a variaty of types. Feb 19, 2019
@Peilonrayz Peilonrayz changed the title Update _make_default_field to work with a variaty of types. Fix schema problems Feb 21, 2019
@dsvensson dsvensson mentioned this pull request Feb 26, 2019
@dsvensson
Copy link

This fixes cases like this, hooray!

class Bar:
 b: str

class Foo:
  a: List[Bar]

But if Bar contains an Enum-field, that fails with:

  File ".../venv/lib/python3.7/site-packages/dataclasses_json/api.py", line 76, in schema
    Schema = build_schema(cls, DataClassJsonMixin, infer_missing, partial)
  File ".../venv/lib/python3.7/site-packages/dataclasses_json/mm.py", line 113, in build_schema
    schema_ = schema(cls, mixin, infer_missing)
  File ".../venv/lib/python3.7/site-packages/dataclasses_json/mm.py", line 98, in schema
    t = build_type(type_, options, mixin, field, cls)
  File ".../venv/lib/python3.7/site-packages/dataclasses_json/mm.py", line 73, in build_type
    return inner(type_, options)
  File ".../venv/lib/python3.7/site-packages/dataclasses_json/mm.py", line 72, in inner
    return field.Field(**options)
AttributeError: 'Field' object has no attribute 'Field'

@Peilonrayz
Copy link
Contributor Author

@dsvensson Looks like an easy fix, I used field rather than fields here.

Could you provide the code that made this error?

@dsvensson
Copy link

@Peilonrayz Oh that was easy. It was indeed just that typo. Too much redacting, but I can provide a test case later today. There's also another test case that ought to be added, for list of classes where one field contains a list of classes. That didn't work before this branch. #57 . More or less extending the last decoder test in the test...nestedsomething..py.

@Peilonrayz
Copy link
Contributor Author

Peilonrayz commented Feb 26, 2019

@dsvensson Yeah I had done that with another fields.Fields, thought I changed all of them after that tho... :(

I've added the test you mention already - test & object. Note, you can reproduce this without the outer most datatype being a list. It's just that the library only allows decoding lists via .schema, otherwise from_json and other code doesn't use the schema code.

@lidatong lidatong self-requested a review February 28, 2019 15:42
Copy link
Owner

@lidatong lidatong left a comment

Choose a reason for hiding this comment

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

Great, thank you so much for reporting the issue and providing this much needed fix! Also thank you for cleaning up the tests and versioning them. I appreciate it a lot.

@lidatong lidatong merged commit 075d58c into lidatong:master Feb 28, 2019
@lidatong
Copy link
Owner

In light of #68 I have updated the code to fix #57, #58, #60, #65, #67 and #68. I have added relevant tests and ensured they pass in Python 3.6 and 3.7. (I've changed the code to allow skipping of the datetime tests.)

thanks!

From this I have moved all schema code into mm so that the marshmallow code is contained. And this is basically just a re-write of mm. I've updated api to accommodate these changes.

Otherwise only core has been changed. This is because marshmallow now loads schema data into the relevant model. (This can be tested against with code in #58). I'm unsure how you'd want the data to be handled, and so I chose to return early so I don't mangle with core too. please verify this is the intended behavior.

will address this separately

This should keep all the previous logic around options, most are in schema such as specifying a default, and handling Optional. The other special piece of logic is when creating a DC, I kept the field_many option. Otherwise build_type now builds all the types you'd previously specified, and a couple more, in a recursive form, defaulting to fields.Field and a warning.

I would like to request that we change how you handle the dataclasses_json.mm_field so that if the value is None we generate one by default. This isn't something you did previously, and so I didn't change this.

yep this makes sense for a future patch

@lidatong
Copy link
Owner

Released in 0.2.2

@lidatong lidatong mentioned this pull request Feb 28, 2019
@Peilonrayz
Copy link
Contributor Author

@lidatong It should be noted that this line should be changed to return fields.Field(**options). As pointed out by @dsvensson otherwise Enum support won't work.

I was waiting for a test case to work against until I committed the fix.

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.

Invalid input type
3 participants