Skip to content

Fix some comparisons #100

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 3 commits into from
Jan 17, 2025
Merged

Fix some comparisons #100

merged 3 commits into from
Jan 17, 2025

Conversation

gburt
Copy link
Contributor

@gburt gburt commented Jan 17, 2025

I noticed that {"==": ["0", 0]} stopped returning true at some point, which is preventing us from upgrading. The JS library returns true for it. I noticed the official tests didn't exercise/test this. so I've opened a PR there to add a bunch more tests of various type-coerced comparisons.

I also added the ability to run the tests with a local tests.json file which I used to fix those tests.

@diegoholiveira
Copy link
Owner

I noticed the official tests didn't exercise/test this. so I've opened a PR there to add a bunch more tests of various type-coerced comparisons.

It's hard to get something merged on the official repos because the original author does not work directly with this anymore. We have two options here:

  1. wait to merge there and them merge it here
  2. inject this into our test suites without depending on the other PR.

I do prefer the second option because it would be much easier to merge this and create a new release.

Also, thanks for the contrib. Very important to tackle the diffs between JS and GO.

@gburt
Copy link
Contributor Author

gburt commented Jan 17, 2025

I noticed the official tests didn't exercise/test this. so I've opened a PR there to add a bunch more tests of various type-coerced comparisons.

It's hard to get something merged on the official repos because the original author does not work directly with this anymore. We have two options here:

  1. wait to merge there and them merge it here
  2. inject this into our test suites without depending on the other PR.

I do prefer the second option because it would be much easier to merge this and create a new release.

Also, thanks for the contrib. Very important to tackle the diffs between JS and GO.

Thanks Diego, and I should have said up top, thank you for this library.

Want me to rev this PR to make/use a stored-in-git copy of the (updated) tests.json file that's run as a new Test func, in addition to the existing HTTP-based test?

@gburt
Copy link
Contributor Author

gburt commented Jan 17, 2025

Done! We run both test suites now

@diegoholiveira diegoholiveira merged commit 3a4ba23 into diegoholiveira:main Jan 17, 2025
6 checks passed
@TotalTechGeek
Copy link

Also commenting here:
@gburt / @diegoholiveira

If y'all get additional test cases, please feel free to send them our way at https://github.com/orgs/json-logic so we can improve our test suite for cross-implementation compatibilty and vote

(😄 Fortunately, it does look like JLE is compatible with these tests)

Also @gburt if you want, I can also send you an invite to the org

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