-
-
Notifications
You must be signed in to change notification settings - Fork 152
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: replace deprecated API in chrono 0.4.35 for converting to time #367
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #367 +/- ##
==========================================
+ Coverage 63.39% 63.88% +0.49%
==========================================
Files 24 25 +1
Lines 1557 1581 +24
==========================================
+ Hits 987 1010 +23
- Misses 570 571 +1 ☔ View full report in Codecov by Sentry. |
and we can add this test
|
@Dirreke Doesn't look like it's doable without some of your refactor. Based on needing to unwrap the error in the |
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.
Looking good, added a few comments.
Basically .unwrap() is ok if there is some explanation as to why it will never happen. (I usually add a comment on the proceeding line.)
For example unwrap is fine if you are passing literals in that you know to be fine. But as soon as you're taking user input we probably need to start handling errors.
Also, just want to note that they really arent your unwraps, just, while were in here we should do the "correct" thing.
.and_local_timezone(Local) | ||
.unwrap() | ||
}; | ||
|
||
#[cfg(not(test))] | ||
let current = Local::now(); | ||
let next_time = TimeTrigger::get_next_time(current, config.interval, config.modulate); | ||
let next_time = | ||
TimeTrigger::get_next_time(current, config.interval, config.modulate).unwrap(); |
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.
need to remove this unwrap and handle the error
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.
Added a match which panics on error. This can be handled more gracefully with changes from #347 which changes where get_next_time
is called to a method that returns a result.
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.
Swapped to unwrap_or
with a 1 second increment
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.
Updated new to return a Result and passed the result up
@estk it looks like Codecov wiped out your access token. Can you try and fix that? |
4263205
to
303d009
Compare
#[derive(Debug, Error)] | ||
enum TimeTrigerIntervalError { | ||
#[error("The 'Seconds' value specified as a time trigger is out of bounds, ensure it fits within an i64: : '{0:?}'")] | ||
Second(TimeTriggerInterval), | ||
#[error("The 'Minutes' value specified as a time trigger is out of bounds, ensure it fits within an i64: : '{0:?}'")] | ||
Minute(TimeTriggerInterval), | ||
#[error("The 'Hours' value specified as a time trigger is out of bounds, ensure it fits within an i64: : '{0:?}'")] | ||
Hour(TimeTriggerInterval), | ||
#[error("The 'Days' value specified as a time trigger is out of bounds, ensure it fits within an i64: : '{0:?}'")] | ||
Day(TimeTriggerInterval), | ||
#[error("The 'Weeks' value specified as a time trigger is out of bounds, ensure it fits within an i64: : '{0:?}'")] | ||
Week(TimeTriggerInterval), | ||
} | ||
|
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.
I dont think it's necessary either. For users, all these parameters are from the item interval
, so just "the interval value....." is enough imo.
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.
I added a comment that was something to the effect of "we should add context to errors". I obviously didnt add enough context, sorry. My point was this: if a user has a huge config with many intervals we need to set up the error handling such that when surfaced to the user, they know which interval was out of bounds.
So we need to:
- Simply add a position discriminant to this error, roughly:
#[derive(Debug, Error)]
enum TimeTriggerIntervalError {
#[error("The '{}' value specified as a time trigger is out of bounds, ensure it fits within an i64: : '{0:?}'")]
OutOfBounds(Position, TimeTriggerInterval)
}
- Ensure that in the caller that TimeTriggerIntervalError is handled and the name of the appender containing it is provided to the user.
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.
I don't think we can access the name anywhere close to the triggers. They're owned in the config module but are not available anywhere within the append module that I can find
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.
Alright I've been staring at this and tried a slew of different things, none of which I think get close to what you're asking for here. I think I need some assistance.
Like I mentioned we don't have access to the appender name, and I'm not really sure what you mean by a Position discriminant. The Error already reports the type of the interval (Seconds, Minutes, Hours, etc.) and I'm not sure what else we can add of value to the user.
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.
I dont think it's necessary to include the appender name. The incorrect interval numbers is enougn to assist users to locate the configuration. Even if they have duplicate incorrect number, they need fix them all because all these numbers will be incorrect for interval
Thanks for the code! |
Just wanted to add some more context here. It makes sense to use failable versions of for example Previously by using the panicking version of these methods the error handling is easy, it just unwinds and gives the user (optionally) a stack trace to go find what happened. Aspirationally we would be able to use the failable methods and direct the user to the data that caused the issue instead of the code that caught the issue. (This is what would be possible in your current approach.) However, if we're going to go thru the trouble of preventing a panic, we need to ensure the usability and debuggability is at least as good as it was when we panicked. As to how we do this... idk, but I'm certain we should alert the user that their input was incorrect instead of simply selecting some arbitrary default. IMO, without holistically rethinking our approach to presenting errors to users it will be difficult to present errors well without panicking. |
#[derive(Debug, Error)] | ||
enum TimeTrigerIntervalError { | ||
#[error("The 'Seconds' time trigger interval value is out of bounds, ensure it fits within an i64: : '{0:?}'")] | ||
Second(TimeTriggerInterval), | ||
#[error("The 'Minutes' time trigger interval value is out of bounds, ensure it fits within an i64: : '{0:?}'")] | ||
Minute(TimeTriggerInterval), | ||
#[error("The 'Hours' time trigger interval value is out of bounds, ensure it fits within an i64: : '{0:?}'")] | ||
Hour(TimeTriggerInterval), | ||
#[error("The 'Days' time trigger interval value is out of bounds, ensure it fits within an i64: : '{0:?}'")] | ||
Day(TimeTriggerInterval), | ||
#[error("The 'Weeks' time trigger interval value is out of bounds, ensure it fits within an i64: : '{0:?}'")] | ||
Week(TimeTriggerInterval), | ||
} | ||
|
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.
Besides, I remember now why I put a specific number here before. The max value of these is not i64::MAX. They should be within i64::MAX after they convert to milliseconds.
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.
Action worthy?
FWIW the get_next_time error would be one that we generate, so I think we can/do/will when I fix the IntervalError provide some data to the user. Bigger picture, we can setup a milestone + branch to start working towards a 2.0 version that includes the API change for multiple rollers and an informative but not panicking solution. |
Closes #367 |
@estk can you review please? |
Hi, any update regarding this PR? |
Replace deprecated time conversion API from chrono crate.
Deprecation occured in v0.4.35.
Work performed by @Dirreke
Separated out by @bconn98
Resolves #366