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 235 allow override formats #240

Merged
merged 4 commits into from
Feb 6, 2018

Conversation

bdowling
Copy link
Contributor

This PR is to fix the need for overriding DEFAULT_FORMATS

I welcome feedback, particularly on the example provided in docs as I am not 100% clear on the validate function in particular, I haven't quite figured out when this is called.

Closes #235

@coveralls
Copy link

coveralls commented Jan 26, 2018

Coverage Status

Coverage remained the same at 97.931% when pulling 520daf5 on bdowling:fix_235_allow_override_formats into 0bdda91 on Yelp:master.

Copy link
Collaborator

@macisamuele macisamuele left a comment

Choose a reason for hiding this comment

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

I like the idea of this PR ... so thanks a lot for helping us in making bravado-core better.

According to what I've seen on the PR I think that this will not solve your original problem.
Let's assume that you receive a response like

{"attr": 1.23456789...}

such that the value of attr will not be "precise" enough as float.
While bravado-core receives this response it will invoke json method on the incoming response (link). The usual implementation of the json method is offloading the call to json or simplejson libraries which are returning a float.
So if you need to keep really high precision it is possible that you do need to create you're "custom" http_client that calls [simple]json.loads with use_decimals=True

if name in formatter.DEFAULT_FORMATS:
return formatter.DEFAULT_FORMATS[name]
format = self.user_defined_formats.get(name)
format = self.user_defined_formats.get(name,
Copy link
Collaborator

Choose a reason for hiding this comment

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

To make it easier to read (and not use too long lines) I would suggest to first fetch the formatter from the user defined formmatters and if nothing is extracted trying to get it from the default formats

format = self.user_defined_formats.get(name)
if format is None:
    format = formatter.DEFAULT_FORMATS.get(name)


Overriding built-in fomrats is also possible with a user-defined format
-----------------------------------------------------------------------
By default 'double' is internally converted to a float in python. This runs
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would mention "double format"


def mydouble_validate(x):
"""Validate input is in fact a decimal type"""
if isinstance(x,Decimal):
Copy link
Collaborator

Choose a reason for hiding this comment

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

The validate method gets the "json" object representing your attribute.
So in your case, it will be something like

...
    type: number
    format: double

It means that type(x) will be a float

Copy link
Contributor Author

@bdowling bdowling Jan 31, 2018

Choose a reason for hiding this comment

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

I realized the schema is actually string(double), so json should leave it alone until it gets marshalled.

@@ -125,3 +126,30 @@ def test_ref(minimal_swagger_dict):
result = to_wire(swagger_spec, int_ref_spec, 999)
assert 999 == result
assert isinstance(result, int)


def test_override(minimal_swagger_dict):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we can simplify this test

Copy link
Contributor Author

@bdowling bdowling Jan 31, 2018

Choose a reason for hiding this comment

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

Git is not seeing this as changed from where you tagged the comment, but I think the new test is a simplified MVT.

@bdowling bdowling force-pushed the fix_235_allow_override_formats branch 2 times, most recently from 72f5280 to de81a52 Compare January 31, 2018 16:20
@bdowling bdowling force-pushed the fix_235_allow_override_formats branch from f54e3f2 to 1d29481 Compare January 31, 2018 18:19
@bdowling
Copy link
Contributor Author

@macisamuele - I re-based squashed and tossed up this PR, let me know if there is anything else.

@bdowling bdowling force-pushed the fix_235_allow_override_formats branch 2 times, most recently from f3aa1e8 to 68f731c Compare January 31, 2018 18:37
@bdowling bdowling force-pushed the fix_235_allow_override_formats branch 2 times, most recently from cda6482 to 12fd40d Compare January 31, 2018 23:42
@bdowling bdowling force-pushed the fix_235_allow_override_formats branch from 12fd40d to 520daf5 Compare January 31, 2018 23:52
@macisamuele macisamuele merged commit ad814f6 into Yelp:master Feb 6, 2018
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.

Format double and overriding default formats
3 participants