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 empty rules object that should return an empty object instead of false #23

Merged
merged 1 commit into from
Nov 21, 2019

Conversation

fifthaxe
Copy link
Contributor

Fix the following bad falsy eval :
=============== >8 ================
rules : {}
data : {}
return : false
=============== 8< ================

As seen in jsonlogic.com playground, a rule defined as an empty object should be evaluated as an empty object instead of a falsy.

@codecov-io
Copy link

codecov-io commented Nov 20, 2019

Codecov Report

Merging #23 into master will increase coverage by 0.15%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
+ Coverage    89.2%   89.35%   +0.15%     
==========================================
  Files           6        6              
  Lines         639      639              
==========================================
+ Hits          570      571       +1     
+ Misses         51       50       -1     
  Partials       18       18

@fifthaxe fifthaxe changed the title fix empty rules map that should return an empty object instead o… fix empty rules map that should return an empty object instead of false Nov 20, 2019
@fifthaxe fifthaxe changed the title fix empty rules map that should return an empty object instead of false fix empty rules object that should return an empty object instead of false Nov 20, 2019
@diegoholiveira
Copy link
Owner

@fifthaxe can you write a test to make sure this bug will never comer back in the future?

thanks anyway :)

@fifthaxe
Copy link
Contributor Author

fifthaxe commented Nov 20, 2019

The tests are based on definitions from http://jsonlogic.com/tests.json, the empty rule object case is not covered. Will you consider that asking for a fix in http://jsonlogic.com/tests.json will be a good solution since this case will then be covered (and tested) by our implementation ?

@diegoholiveira
Copy link
Owner

The tests are based on definitions from http://jsonlogic.com/tests.json, the empty rule object case is not covered. Will you consider that asking for a fix in http://jsonlogic.com/tests.json will be a good solution since this case will then be covered (and tested) by our implementation ?

Sure. This is too general and would be great to be in the tests.json, but meanwhile, we can solve it here, no need to wait until it be available at tests.json.

@fifthaxe
Copy link
Contributor Author

Ok so it seems we are done with this PR.
I also have opened an issue to Jeremy Wadhams. Take a look at it jwadhams/json-logic#21.

@diegoholiveira diegoholiveira merged commit dc98137 into diegoholiveira:master Nov 21, 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.

3 participants