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

Skip title if either fields are None #93

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

yeralin
Copy link
Contributor

@yeralin yeralin commented Oct 3, 2019

Addresses #87

Because of lack of support of doubly (or more) nested schemas, some titles get rendered as null which breaks JSON schema validation. This PR simply skips title if field.attribute or field.name are None

@coveralls
Copy link

coveralls commented Oct 3, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling eab1de0 on yeralin:fix-doubly-nested-null-titles into 47418df on fuhrysteve:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.6%) to 98.44% when pulling 4f563ad on yeralin:fix-doubly-nested-null-titles into 307a911 on fuhrysteve:master.

@yeralin yeralin force-pushed the fix-doubly-nested-null-titles branch from 2cc2ef8 to 72234a2 Compare October 3, 2019 19:20
@yeralin
Copy link
Contributor Author

yeralin commented Oct 3, 2019

The build is failing because tox is installing marshmallow-jsonschema==0.7.0 that does not contain recently merged change from #86

After a new version is released, the build is supposed to pass.

@yeralin yeralin force-pushed the fix-doubly-nested-null-titles branch from 72234a2 to 01d42ac Compare October 28, 2019 19:15
@yeralin
Copy link
Contributor Author

yeralin commented Oct 29, 2019

Hmmm even after a new release, the build is still failing... I also added an extra test case, but the coveralls are still -1.8% :(

@atmo
Copy link
Contributor

atmo commented Dec 28, 2019

Hi @yeralin! The build is failing because of a subtle bug that was introduced in previous PRs. See #102 for details. I'm afraid that we'll have to wait until that PR fixes master, and only then you can rebase your branch and have clean builds.

I think that your solution works just fine: it maintains backward compatibility and in my opinion we will need to drop Python 2 support and Marshmallow 2 support very soon anyway.

@yeralin yeralin force-pushed the fix-doubly-nested-null-titles branch from 01d42ac to dcf4b47 Compare March 2, 2020 14:18
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.

3 participants