-
Notifications
You must be signed in to change notification settings - Fork 23
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
Proper Error
types for the crate
#386
Conversation
2a8327e
to
3e559a1
Compare
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
3e559a1
to
7e85052
Compare
Signed-off-by: Alex Snaps <[email protected]>
cffdc82
to
539d53c
Compare
Signed-off-by: Alex Snaps <[email protected]>
539d53c
to
443ef8c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
} | ||
} | ||
|
||
impl From<Infallible> for LimitadorError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
out of curiosity, why do we need to add this implementation?
When I apply this patch, it is not needed.
diff --git a/limitador/src/errors.rs b/limitador/src/errors.rs
index d590aaf..3de262e 100644
--- a/limitador/src/errors.rs
+++ b/limitador/src/errors.rs
@@ -1,6 +1,5 @@
use crate::limit::ConditionParsingError;
use crate::storage::StorageErr;
-use std::convert::Infallible;
use std::error::Error;
use std::fmt::{Display, Formatter};
@@ -43,9 +42,3 @@ impl From<ConditionParsingError> for LimitadorError {
LimitadorError::InterpreterError(err)
}
}
-
-impl From<Infallible> for LimitadorError {
- fn from(value: Infallible) -> Self {
- unreachable!("unexpected infallible value: {:?}", value)
- }
-}
diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs
index 94b95b1..511f083 100644
--- a/limitador/src/limit.rs
+++ b/limitador/src/limit.rs
@@ -1080,7 +1080,7 @@ mod tests {
limit1.namespace.clone(),
limit1.max_value + 10,
limit1.seconds,
- limit1.conditions.clone(),
+ vec!["req.method == 'GET'"],
limit1.variables.clone(),
)
.expect("This must be a valid limit!");
Not arguing to change it, just curiosity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you pass an iterator of Condition
which are TryFrom
... but Infallible
.
It'll go away, when I deal with the rest of the refactoring, but yeah... It is perfectly fine to pass Condition
s right in the Limit
that you know cannot fail.
Fixes #131
This is a first pass at this.
LimitadorError::InterpreterError
will "only" be used as a mean to cover for runtime errors, e.g. an expression that can't be resolved (see #385). Also, when doing that work, we should be able to return to aLimit::new -> Self
over the current result and clean that API up a bit.Pretty sure other cases will emerge for
Limitador::Error
as we move towards v2.0 of the server, but hopefullyv1
for the crate 🤞