Generalise slicer #72
Conversation
…not "int") ti tuple when trying to slice pydub audio
There was a problem hiding this comment.
Hey @RalphHuber, I found this a fun case study to review, as I had similar inclinations to use datetime and timedelta. I think you're right about the clock time limitation - the timestamps I've been given by @ProfessorTerrence have sometimes been specified as 105:40 without a separate hour component, and it would be good to have our CLI robust to that to save me and others a little math when we're working through a large list of files or timestamps. So I think I'll close this one.
I feel like there's probably a lot of interesting prior art here with how other Python projects have parsed and handled timestamps for audio and video recordings. @heymanpreet you may find some nice time parsing implementations as you work through #68, and you may want to study this PR's diff and our comments to avoid running into the same challenges.
| audio_start_time_list = audio_slice_start_time.split(":") | ||
| audio_end_time_list = audio_slice_end_time.split(":") | ||
| @dataclass | ||
| class SlicerInput: |
There was a problem hiding this comment.
I like the dataclass! Adding a docstring to help Sphinx autodoc:
| class SlicerInput: | |
| class SlicerInput: | |
| """ | |
| A dataclass representing input for the slicer. | |
| Attributes: | |
| path (Path): The path to the input file. | |
| start_time (timedelta): The start time for slicing. | |
| end_time (timedelta): The end time for slicing. | |
| """ |
| try: | ||
| sliced_audio = slice_audio(audio, argv.start_time, argv.end_time) | ||
| except ValueError: | ||
| raise ValueError("Error! Audio slice input params cannot be greater than original audio size. Please try again with correct parameters.") |
There was a problem hiding this comment.
Cool, I like what you did here. Extending the value error message is good. In the future we may want something like an exceptions.py with custom exceptions.
| # Converting audio start and end times in msecs | ||
| audio_start_time = convert_audio_time_to_msec(audio_start_time_list) | ||
| audio_end_time = convert_audio_time_to_msec(audio_end_time_list) | ||
| def convert_time_input_to_time_delta(time_input: str): |
There was a problem hiding this comment.
I think your instinct to reuse something like timedelta here was great. I empathize with the challenges you ran into: parsing time strings is not easy!
I can see how you came to the conclusion. You might need a bespoke parser. I feel like someone must have written a moment.js equivalent for Python by now. Maybe there is something like that.
There was a problem hiding this comment.
Thanks Audrey. Yes it started out quite clean, and then slowly started getting less and less haha. Unfortunate as it could have been a fairly simple solution. It's quite surprising a time concept on its own doesn't really exist in the std library, only when associated with date time. Ah nice, I'll see if there's a library about, I feel like there must be.
Thanks for your review, massively appreciate it
|
Thanks for tagging here @audreyfeldroy, I will have look at time parsers for python. |
|
Thanks @heymanpreet, look forward to seeing what you come up with |
|
Thanks for the feedback @audreyfeldroy . Yea, frustrating that it ended up getting limited to that. I don't think there's a way round it if trying to make use of datetimes strptime() function. If nothing else, can probably steal strptimes implementation |
audreyfeldroy
added
the
hacktoberfest-accepted
Added types, raises errors
separated parsing input, slicing, and exporting. Separated creation from use
Potential Improvements:
tried to generalise with time delta so we werent forced to pass ints around
tried to make use of datetimes string formatting to easily allow for hours, and be able to make changes to expected input. This sort of worked; expected string format is now a constant that we can change. However, now we are limited to clock time (e.g. number of minutes cannot exceed 60). I feel using datetime has actually been more restrictive than I first though, probably should have written a bespoke parser.