Skip to content

fix: compare matches to inbound params, parse number 0 #67

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

danwalkerdev
Copy link

@danwalkerdev danwalkerdev commented Apr 3, 2025

Fix for issue #66

  • compare matchable params to inbound.params
  • allow the number 0 falsy parameters to be parsed and provided in the props

@mateothegreat
Copy link
Owner

We're doing some weird stuff in the condition checks.

Getting false positives:

image

Same condition under main:

image

@mateothegreat mateothegreat self-assigned this May 25, 2025
@mateothegreat mateothegreat added acknowledged Acknowledged, work in progress blocked and removed acknowledged Acknowledged, work in progress labels May 25, 2025
@danwalkerdev
Copy link
Author

danwalkerdev commented May 26, 2025

nice catch - it's a little confusing because evaluators can return different types (mainly for the regexp option)

I think checking the type is as expected (from the previously marshalled value) can allow us to let falsy values through if they are of the expected type. I think it's the easiest way of distinguishing whether false from the evaluator means "doesn't match" vs "matches to boolean false", without restructuring what the evaluator returns.

Do you think we need any further checks for the array return types?

Incidentally, I also noticed something like the following didn't parse as expected

test("regexp groups", () => {
    expect(new Query("first=hello:world")
      .test(new Query({
        first: /^(\w+):(\w+)$/,
      })))
      .toEqual({
        condition: 'exact-match',
        ...
      })
})

either with a comma or colon separator. might be a separate thing to look at

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants