Skip to content

Fix number validator #134

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

Merged
merged 2 commits into from
May 21, 2019
Merged

Fix number validator #134

merged 2 commits into from
May 21, 2019

Conversation

dz0ny
Copy link
Contributor

@dz0ny dz0ny commented May 15, 2019

The integer_types is always a tuple. When checking
if an instance is a number it fails because it's doing a comparison against a tuple
instead of real type.

➜ python -c "from six import integer_types;import sys;print(integer_types);print(sys.version)"
(<type 'int'>, <type 'long'>)
2.7.16 (default, Apr 6 2019, 01:42:57)
[GCC 8.3.0]

➜ python3 -c "from six import integer_types;import sys;print(integer_types);print(sys.version)"
(<class 'int'>,)
3.7.3 (default, Apr 3 2019, 05:39:12)
[GCC 8.3.0]

And spec defines a number as both int and float https://swagger.io/docs/specification/data-models/data-types/#numbers so both validators need to support both types.

@dz0ny dz0ny force-pushed the fix/number_parsing branch from c087ea6 to 56ecb61 Compare May 15, 2019 16:52
@codecov
Copy link

codecov bot commented May 15, 2019

Codecov Report

Merging #134 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #134   +/-   ##
=======================================
  Coverage   96.33%   96.33%           
=======================================
  Files          54       54           
  Lines        1474     1474           
=======================================
  Hits         1420     1420           
  Misses         54       54
Impacted Files Coverage Δ
openapi_core/schema/schemas/models.py 95.2% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f274836...63f3ffb. Read the comment docs.

@dz0ny dz0ny force-pushed the fix/number_parsing branch from 56ecb61 to 69a580a Compare May 15, 2019 16:59
The `integer_types` is always a tuple. When checking
if an instance is a number it fails because it's doing a comparison against a tuple
instead of real type.

➜ python -c "from six import integer_types;import sys;print(integer_types);print(sys.version)"
(<type 'int'>, <type 'long'>)
2.7.16 (default, Apr  6 2019, 01:42:57)
[GCC 8.3.0]

➜ python3 -c "from six import integer_types;import sys;print(integer_types);print(sys.version)"
(<class 'int'>,)
3.7.3 (default, Apr  3 2019, 05:39:12)
[GCC 8.3.0]

And spec defines a number as both int and float https://swagger.io/docs/specification/data-models/data-types/#numbers so both validators need to support both types.
@dz0ny dz0ny force-pushed the fix/number_parsing branch from 69a580a to 3339e13 Compare May 15, 2019 17:00
raise InvalidSchemaValue("Value {value} is not of type {type}", value, self.type)

return float(value)
Copy link
Collaborator

@p1c2u p1c2u May 21, 2019

Choose a reason for hiding this comment

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

Hmm This should stay as Python representation for number is float.

Copy link
Collaborator

@p1c2u p1c2u May 21, 2019

Choose a reason for hiding this comment

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

OpenAPI has two numeric types, number and integer, where number includes both integer and floating-point numbers. An optional format keyword serves as a hint for the tools to use a specific numeric type:

Ah yeah you're right. We need format unmarshalling then.

@p1c2u p1c2u merged commit 98f72bf into python-openapi:master May 21, 2019
bjmc pushed a commit to bjmc/openapi-core that referenced this pull request Jun 12, 2019
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.

2 participants