-
Notifications
You must be signed in to change notification settings - Fork 57
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
feat: add from_unix_timestamp function #277
Conversation
src/stdlib/from_unix_timestamp.rs
Outdated
} | ||
|
||
fn type_def(&self, state: &state::TypeState) -> TypeDef { | ||
self.value |
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.
This is not quite right.
.fallible_unless(Kind::timestamp())
makes the type definition fallible unless the input kind is a timestamp. There are a few issues with this. First, the VRL compiler automatically checks function argument types, so you don't need to worry about checking those here. Second,timestamp
is an invalid type here anyway.with_kind
completely overwrites the original kind (self.value
) here, so there's no need to useself.value
at all
There's 2 parts to a type definition, so lets look at the individually:
kind
: This is the actual type returned by the function. This function will always return a timestamp, so it can just be hard-coded toKind::timestamp()
.fallibility
: This is a boolean, saying if the function can fail (at runtime) or not. There's an interesting edge case here that I'll talk about below, but in general this function can always potentially fail. If you want to just hard-code this to fallible I'd be okay with that, so the entire type def becomesTypeDef::timestamp().fallible()
If we can prove at (VRL) compile time that a function will not fail, it's nice to change the type definition in that case so the user doesn't have to try to handle errors that can't happen. This usually applies to arguments where the most common pattern is to pass in a literal, such as the "unit" argument. However, even if we did that, the timestamp conversion itself can usually fail. There is a single edge case where it is known that nanoseconds
is hard-coded as the unit, that this function is infallible. If you want to try to handle that case, I can give you an example to follow, however that seems like it's a small enough edge case that it might not really be helpful to users and might just end up confusing them more.
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.
Thank you, this is very helpful! Regarding the nanoseconds edge case, I am inclined to agree it won't really help users.
b897728
to
e7f2bff
Compare
The code changes look good now. Please add a line to |
closes: #188
Contains unit tests. Also, tested with REPL:
This introduces some code duplication with
stdlib/to_timestamp.rs
but the latter will be deprecated soon.