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.

Sign up for GitHub

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

Jump to bottom

fix: use uint for (no_)expect_ids #8

Merged
merged 1 commit into from
May 9, 2024
Merged

fix: use uint for (no_)expect_ids #8

merged 1 commit into from
May 9, 2024

Conversation

theseion
Copy link
Collaborator

@theseion theseion commented May 6, 2024

No description provided.

@theseion theseion requested a review from fzipi May 6, 2024 04:59
@theseion
Copy link
Collaborator Author

theseion commented May 9, 2024

ping.

@fzipi
Copy link
Member

fzipi commented May 9, 2024

Just wanted to reference this discussion in Coraza about uint vs int.

@theseion
Copy link
Collaborator Author

theseion commented May 9, 2024

In this case at least, uint helps with correctness. We never expect a rule number to be negative and we don't check for it anywhere. So letting the parser handle the type checking is just a simple quality of life improvement for me.

@theseion theseion merged commit 48cda85 into coreruleset:main May 9, 2024
1 check passed
@theseion theseion deleted the more-changes branch May 9, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@M4tteoP M4tteoP M4tteoP approved these changes

@fzipi fzipi fzipi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@theseion @fzipi @M4tteoP