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

Range Check should allow negative numbers #220

Open
chrisknoll opened this issue Feb 14, 2025 · 1 comment
Open

Range Check should allow negative numbers #220

chrisknoll opened this issue Feb 14, 2025 · 1 comment

Comments

@chrisknoll
Copy link
Collaborator

The RangeCheckFactory uses the following logic:


                .orElse( r -> match(r)
                        .when(Comparisons::isStartNegative)
                        .then(() -> warning.accept(WARNING_START_IS_NEGATIVE))
                        .when(x -> Objects.isNull(x.value))
                        .then(() -> warning.accept(WARNING_EMPTY_START_VALUE)));

But there should be no problem with negative numbers, instead it should just warn if the value is empty.

In addition, this code looks like it should push warnings, but this is resulting in an 'error' type. But we'll solve this by removing the null check , but still it seems to be an issue around warnings being treated as errors.

@chrisknoll
Copy link
Collaborator Author

I answered my question about why it's showing up as an error. the default severity is set to Critical.

I'm looking through the code, and it is fairly confusing how everything comes together between the Interfaces, paramaterized types, default severity, etc. You have to keep multiple things in your head at once in order to understand a simple check operation. One thing that is a challenging (it appears) is a checker producing errors and warnings in the same class, it seems like you have to specify that all checkers have one severity level and I think in order to get some checks to return warnings and others errors, you need distinct checker classes that will report one or the other.

For example (related to this issue) if i want to have some checks in Range to return errors, and others to be warnings, I'd have to define 2 separate factories (one for the warnings, one for the errors). I feel that the API would have been simpler to understand if it worked like a logging api where when you publish a check, you specify if the check is a warning or an error.

The way it's implemented, I have to potentially look up an inheritance tree to understand if the class is warning or erroring.

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

No branches or pull requests

1 participant