craffel / pretty-midi

Utility functions for handling MIDI data in a nice/intuitive way.
MIT License
856 stars 151 forks source link

Allow pre-loaded mido.MidiFile objects to be passed as an argument to PrettyMIDI #241

Closed a-pillay closed 6 months ago

a-pillay commented 7 months ago

This closes #200 and #201

craffel commented 7 months ago

We actually need to put the new arg at the end of the args list to avoid breaking code that uses positional arguments.

a-pillay commented 7 months ago

@craffel I agree.

I have modified mido_object to be the last argument in PrettyMIDI. Please let me know if anything else needs to be addressed!

craffel commented 7 months ago

Need to change argument order in docstring 😅

a-pillay commented 7 months ago

I was considering setting the mido_object description as the last one in the argument docstring, but proceeded with keeping it immediately after midi_file to highlight how the former will be prioritized.

However, I can set the mido_object description as the last one (in sync with the argument order) if you could confirm that it sounds good. I find both options equally logical.

craffel commented 6 months ago

Docstring argument lists must match the order of the arguments.

a-pillay commented 6 months ago

Sounds good! I have updated the docstrings as suggested. Thanks for putting up with my repeated requests for clarifications!

craffel commented 6 months ago

Can you raise a ValueError if both a filename and a mido object are provided? This should not work silently because the correct behavior is ambiguous. The logic would look like

if mido_object is not None and midi_file is not None:
   raise ValueError("A midi_file and midi_object cannot both be provided.")

and then the rest of the logic can proceed assuming that only one of them has been provided (e.g. you don't need an elif).

a-pillay commented 6 months ago

Done! Now, a ValueError will be raised if both mido_object and midi_file arguments are provided at the same time. Also, updated the concerned docstring and unit test to reflect the same. I have also added tests for verifying these ValueErrors but since the existing tests don't use a toolkit like unittest, I have implemented them in an alternate way.

craffel commented 6 months ago

Thanks!