SubRipTime.__init__ should maybe cast the arguments to int or float (aka “TypeError: '>' not supported between instances of 'SubRipTime' and 'dict'” in slice()) #78
then you'll get a rather cryptic error: TypeError: '>' not supported between instances of 'SubRipTime' and 'dict'. This is caused by a different error: TypeError: '>' not supported between instances of 'str' and 'int' in ComparableMixin._compare. Which in turn means that the ordinal field in one of the objects is a string.
The root cause is that, when passed strings as the arguments, the SubRipTime constructor multiplies them by HOURS_RATIO, MINUTES_RATIO and SECONDS_RATIO respectively, and adds them all together, silently resulting in a long-ass string instead of a number.
To either handle the use-case or make the output more informative, it would be prudent to convert the arguments to numbers, or to explicitly forbid non-number arguments. IMO the first approach is better, especially since Python itself would then complain if the arguments really don't contain numbers. One other question to decide is whether the constructor should accept fractional times and thus convert to float and not just int. Milliseconds should probably be integers, but I might want to cut e.g. 1.5 hours into a film. Some workflows involving arithmetic might even produce fractional milliseconds, which of course should still be cut to integers after parsing.
Alternatively, or in addition, you might want to pass exceptions through in ComparableMixin._compare instead of returning NotImplemented: firstly, AttributeError and TypeError may have more possible causes when calling _cmpkey than just the two envisioned cases. Secondly, use of a mixin suggests more complex workflows than plain comparison of two values—as in the very case of SubRipTime—while Python's resulting message is rather unenlightening. So passing exceptions through seems to be more informative, as they would properly indicate invalid use of ComparableMixin.
If you receive some times as strings, split them into parts and try calling
SubRipFile.slicewith a dict of those parts, e.g.:subs.slice(starts_after={'minutes': '11', 'seconds': '22'})then you'll get a rather cryptic error:
TypeError: '>' not supported between instances of 'SubRipTime' and 'dict'. This is caused by a different error:TypeError: '>' not supported between instances of 'str' and 'int'inComparableMixin._compare. Which in turn means that theordinalfield in one of the objects is a string.The root cause is that, when passed strings as the arguments, the SubRipTime constructor multiplies them by HOURS_RATIO, MINUTES_RATIO and SECONDS_RATIO respectively, and adds them all together, silently resulting in a long-ass string instead of a number.
To either handle the use-case or make the output more informative, it would be prudent to convert the arguments to numbers, or to explicitly forbid non-number arguments. IMO the first approach is better, especially since Python itself would then complain if the arguments really don't contain numbers. One other question to decide is whether the constructor should accept fractional times and thus convert to float and not just int. Milliseconds should probably be integers, but I might want to cut e.g. 1.5 hours into a film. Some workflows involving arithmetic might even produce fractional milliseconds, which of course should still be cut to integers after parsing.
Alternatively, or in addition, you might want to pass exceptions through in
ComparableMixin._compareinstead of returningNotImplemented: firstly, AttributeError and TypeError may have more possible causes when calling_cmpkeythan just the two envisioned cases. Secondly, use of a mixin suggests more complex workflows than plain comparison of two values—as in the very case of SubRipTime—while Python's resulting message is rather unenlightening. So passing exceptions through seems to be more informative, as they would properly indicate invalid use of ComparableMixin.