-
Notifications
You must be signed in to change notification settings - Fork 42
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
Ensure type-safe serialization/deserialization of total time #405
Conversation
Under unknown circumstances, the extra `total_time` argument passed to instances of jobs is serialized/deserialized as a string instead of a float. That causes a `TypeError` due to an impossible type conversion. Now the `total_time` argument is coerced to a float number prior its serialization and deserialization, respectively.
I have signed the CLA! |
"total_time" => total_time, | ||
"total_time" => total_time.to_f, |
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 one should not be necessary if we're always casting it on deserialization, right?
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.
Yeah, not necessary. Simply added for completeness.
) | ||
end | ||
|
||
def deserialize(job_data) # @private | ||
super | ||
self.cursor_position = job_data["cursor_position"] | ||
self.times_interrupted = job_data["times_interrupted"] || 0 | ||
self.total_time = job_data["total_time"] || 0 | ||
self.total_time = job_data["total_time"].to_f || 0.0 |
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.
Should we use Float(...)
here, instead of .to_f
? It has better error messages when it's fed things that are not floats. We do need to be careful with nil
in that case:
self.total_time = job_data["total_time"].to_f || 0.0 | |
self.total_time = Float(job_data["total_time"] || 0.0) |
Apologies, you'll need to rebase to get CI to run properly, as there were some lint error that I fixed recently. |
Closed in favour of #417 |
Under unknown circumstances, the extra
total_time
argument passed to instances of jobs is serialized/deserialized as a string instead of a float. That causes aTypeError
due to an impossible type conversion.Now the
total_time
argument is coerced to a float number prior its serialization and deserialization, respectively.Fixes #396.