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 OR expressions with true on the left hand side #15

Merged
merged 2 commits into from
Mar 31, 2016

Conversation

iconara
Copy link
Contributor

@iconara iconara commented Jan 11, 2016

This fixes a bug where foo || bar where foo is true fails with NoMethodError: undefined methodempty?' for true:TrueClass`.

TrueClass does not respond to `#empty?`, which makes `foo || bar` where foo resolves to true fail.
@trevorrowe
Copy link
Contributor

Thanks for the pull request and fix. @jamesls What are your feelings about expanding the compliance tests? Should be this included in the default suite, or should these be moved to Ruby implementation specific tests? The root of the issue is how the Ruby implementation used .empty? method in was being invoked to determine falsey-ness.

@trevorrowe
Copy link
Contributor

@jamesls @iconara I just took a look at the jmespath/jmespath.test repository, and there appears to be quite a few new tests. Included is a new suite for dealing with booleans. It looks like the new booleans.json test suite would cover this feature and not require the other test change.

@iconara
Copy link
Contributor Author

iconara commented Jan 11, 2016

Getting better (and standardized) tests sounds like a great thing. Is there any easy way to link it in so that it will remain up to date?

@jamesls
Copy link
Member

jamesls commented Jan 11, 2016

@trevorrowe Yep, I believe these should now be accounted for in jmespath.test. There's a recent JEP that improved general boolean support in jmespath (jmespath/jmespath.site#16).

I would like to get a more automated system in place for when new tests are added to jmespath.test to better communicate test suite updates. I tried using submodules for a while in jmespath.py, but that ended up being more trouble than it's worth.

@trevorrowe trevorrowe merged commit 0884f19 into jmespath:master Mar 31, 2016
@trevorrowe
Copy link
Contributor

Sorry for the slow response on this. I merged your pull request locally, and then updated the compliance spec tests. I've got a few other things to wrap up and then I'll try to get a new release cut.

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.

None yet

3 participants