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

Allow providing custom fields mappings #22

Closed
wants to merge 2 commits into from
Closed

Allow providing custom fields mappings #22

wants to merge 2 commits into from

Conversation

s0undt3ch
Copy link

This will allow sub-classing fields and allow marshmallow_jsonschema to
render them as the parent class would.

@coveralls
Copy link

coveralls commented Jan 13, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 96a52c8 on s0undt3ch:master into 6f51771 on fuhrysteve:master.

@fuhrysteve
Copy link
Owner

fuhrysteve commented Mar 29, 2017

Wow this is great and thanks for adding the test case - sorry I didn't get around to this sooner. So I've been thinking about this a little, and while I like the new functionality, I'm a bit hesitant to encourage behavior that basically isn't thread-safe when it seems like there's a good alternative. What if the implementation looked more like this:

# Documented and recommended approach for general use
json_schema = JSONSchema(json_schema_type_mappings={fields.Color: text_type})
json_schema.dump(MySchema())

If you really want to have this globally, you can always do this somewhere global:

JSONSchema.JSONSchemaTypesMapping[fields.Color] = text_type

Or, you could simply extend the base JSONSchema class, which would be the recommended way of adjusting the behavior for global use:

class JSONSchemaExtended(JSONSchema):
    pass

JSONSchemaExtended.JSONSchemaTypesMapping[fields.Color] = text_type

Let me know what you think

@s0undt3ch
Copy link
Author

So, are you still OK of I remove the classmethod but rely on the class level attribute to either subclass or update the class attribute?

@coveralls
Copy link

coveralls commented Nov 24, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 593c752 on s0undt3ch:master into e6a6bce on fuhrysteve:master.

@coveralls
Copy link

coveralls commented Nov 24, 2017

Coverage Status

Coverage decreased (-0.7%) to 99.301% when pulling 18ab94d on s0undt3ch:master into e6a6bce on fuhrysteve:master.

@coveralls
Copy link

coveralls commented Nov 24, 2017

Coverage Status

Coverage decreased (-0.7%) to 99.291% when pulling d3009c8 on s0undt3ch:master into e6a6bce on fuhrysteve:master.

@coveralls
Copy link

coveralls commented Nov 24, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling dba3fc8 on s0undt3ch:master into e6a6bce on fuhrysteve:master.

@s0undt3ch
Copy link
Author

Could you please take another look and consider this again? Thanks!

This will allow sub-classing fields and allow marshmallow_jsonschema to
render them as the parent class would.
@s0undt3ch s0undt3ch changed the title Allow registering field subclasses. Allow providing custom fields mappings Nov 24, 2017
@coveralls
Copy link

coveralls commented Nov 24, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 2b82154 on s0undt3ch:master into e6a6bce on fuhrysteve:master.

@s0undt3ch
Copy link
Author

Ok, this final solution seems like the least intrusive

@coveralls
Copy link

coveralls commented Nov 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 1f7c1de on s0undt3ch:master into e6a6bce on fuhrysteve:master.

@s0undt3ch
Copy link
Author

Polite ping.

2 similar comments
@s0undt3ch
Copy link
Author

Polite ping.

@s0undt3ch
Copy link
Author

Polite ping.

@atmo
Copy link
Contributor

atmo commented Jul 22, 2019

Hi @s0undt3ch ! I would like to recommend using a different approach here. Have a look at how apispec module handles the same problem: it defines a decorator map_to_openapi_type which modifies the global mapping.

I think that decorator is a more convenient and readable solution. Could you please rework your PR to use the decorator instead and resolve the conflicts? See also apispec docs for custom fields.

@atmo atmo mentioned this pull request Jul 22, 2019
@s0undt3ch s0undt3ch closed this by deleting the head repository Jun 3, 2024
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.

4 participants