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

add support for floating point comparisons #43

Merged
merged 1 commit into from
Apr 3, 2018
Merged

add support for floating point comparisons #43

merged 1 commit into from
Apr 3, 2018

Conversation

cgeers
Copy link

@cgeers cgeers commented Mar 22, 2018

I ran across this and discovered only integers were supported in comparisons. The specification supports floating point numbers. Simply expand the scope to the Numeric class to validate any number that is comparable.

@cgeers
Copy link
Author

cgeers commented Mar 30, 2018

@trevorrowe any thoughts here?

@trevorrowe
Copy link
Contributor

Sorry for the slow response. The change seems reasonable.

The spec compliance tests are unfortunately quite a bit out of date. They were originally pulled from here: https://github.com/jmespath/jmespath.test/tree/master/tests

Its been long enough that they have drifted quite a bit. My biggest concern is that if/when I update the bundled compliance tests that your change will be lost and the relevant code fix would not be covered in tests. This would leave us open to a possible future regression. Would it be possible to add another test case, outside of the spec/compliance fixtures to test this case? If so, then I would be very confident to merge your change and to publish an updated release.

@cgeers
Copy link
Author

cgeers commented Apr 1, 2018

no worries. I've added an example outside of spec/compliance.

@trevorrowe trevorrowe merged commit a93b9d3 into jmespath:master Apr 3, 2018
@trevorrowe
Copy link
Contributor

It's been merged and I'll cut a release shortly. Thanks!

@trevorrowe
Copy link
Contributor

Version 1.4.0 is now public. Thank you for the contribution!

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