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 in:A,B,C-rule validation by adding spread-support #67

Conversation

christoph-kluge
Copy link

This MR adds spread-support for the in-rule.

I've discovered that in:A,B,C within validate.ts does only validate the first option (A) inside the list. This is due to the spread-usage ...[...rule.params, context] inside validate.ts.

const isRuleValid = await rule.callback(
    check.value,
    ...[...rule.params, context]
);

basically calls the in-rule like that:

in('B', ...['A', 'B', 'C', {}])

This is used inside validate() function which uses ""...[...rule.params]"". This is using spread of all params, meaning that during the validate() it was only resolving the first item
Copy link

sonarqubecloud bot commented Feb 3, 2025

@ozziest ozziest changed the base branch from main to develop February 3, 2025 16:08
@ozziest ozziest self-requested a review February 3, 2025 16:08
@ozziest ozziest added the bug Something isn't working label Feb 3, 2025
Copy link
Member

@ozziest ozziest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christoph-kluge thank you very much for the contribution. 👊

I'll be released in the next version, probably today. 🚀

Copy link
Member

@ozziest ozziest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christoph-kluge I reviewed the PR deeply, and tried to run tests, and noticed the issue is more complicated.

I've created a ticket about this: #70

Your contribution is very valuable. 🙏 It helped me to understand the issue.
But I have to refuse this PR, unfortunately, since the issue is bigger than I thought.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants