Skip to content
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

Deprecate elapsed_time_to_string accepting time as milliseconds #4862

Closed
pekkaklarck opened this issue Sep 9, 2023 · 1 comment
Closed

Comments

@pekkaklarck
Copy link
Member

The robot.utils.elapsed_time_to_string utility function currently accepts the elapsed time to be formatted as an integer or float representing milliseconds. This made sense earlier, because the elapsedtime attribute of our result model objects contained milliseconds as well, but as part of #4258 we now use elapsed_time that contains a timedelta. timedelta has microsecond precision and also directly support seconds with their total_seconds() method. Our utils working with milliseconds doesn't thus make much sense anymore.

The elapsed_time_to_string was already enhanced to accept the elapsed time as a timedelta, but I believe we should change it to consider ints/floats as seconds as well. Although it's unlikely this function is used outside our code base, that is certainly possible and just changing the behavior wouldn't be good. I thus believe we should deprecate the old behavior first. The problem is that the int/float value itself cannot tell should it be interpreted as seconds or milliseconds, so we need to add a new argument to control that. If the new argument is used, I believe it could be seconds=True, then the value is interpreted as seconds. If it's not used, we can interpret it as milliseconds and emit a deprecation warning.

@pekkaklarck
Copy link
Member Author

It's a bit annoying we need this kind of an utility function in the first place now that we in general use timedelta for representing elapsed times. There just doesn't seem to be any easy way to format timedelta in format hh:mm:ss[.mil]. With timestamps, that are nowadays represented as datetime, we can typically simply use datetime.isoformat().

Another thing to consider is changing the precision of elapsed_time_to_string(). As mentioned above, timedelta has microsecond precision and there might be needs to support that with elapsed_time_to_string as well. Instead of adding new include_micro argument, we probably should add timespec similarly as datetime.isoformat() has. That's a topic for another issue, though.

@pekkaklarck pekkaklarck self-assigned this Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant