Improve duration parsing and error feedback #115
Conversation
Eisenwave
force-pushed
the
improve-duration-handling
branch
2 times, most recently
from
04bf3ee
to
a25df20
Compare
Eisenwave
force-pushed
the
improve-duration-handling
branch
from
a25df20
to
d13bf9d
Compare
| const [_, n, unit] = match; | ||
| const unit_millis = millis_of_time_unit(unit); | ||
| if (unit_millis == null) { | ||
| throw new ParseError(`Invalid time unit in duration: ${unit}`); |
There was a problem hiding this comment.
It might be best to throw this where you return null; from millis_of_time_unit
There was a problem hiding this comment.
I did consider that, and I prefer keeping it because it makes millis_of_time_unit more reusable. There's no reason why it innately needs to throw, and we could use it later to e.g. verify whether a time unit is valid with millis_of_time_unit(...) == null.
| // Returns the corresponding duration in milliseconds, | ||
| // or null for permanent duration (returned if the given duration is null). | ||
| // Throws ParseError when parsing fails. | ||
| export function parse_nullable_duration(duration: string | null) { |
There was a problem hiding this comment.
Take or leave suggestion: This can probably go away now
There was a problem hiding this comment.
There are some places that call the plain parse_duration. I think it makes sense to keep these functions separate because it isolates the "null is permanent" decision into a convenience function, making parse_duration slightly more reusable and safer.
One could run into the case where they're getting string | null from some other function where null represents an error for instance, and plugging that straight into a parse_duration(string | null) function would silence what should be a type error.
|
Closes #114.
I've had some issues running checks, so this is probably going to fail some lint checks, but here's the idea for now.