craffel / pretty-midi

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

Allow pathlib.Path as input for PrettyMIDI init #225

Closed almostimplemented closed 1 year ago

almostimplemented commented 1 year ago

Minor convenience PR to accept pathlib.Path when passing a filename as input to pretty_midi.PrettyMIDI.

craffel commented 1 year ago

I'm out of touch with best practices - is this better than just requiring the user to pass str(path) on their local Path instance (path in my example)? Like, is it best practice to just be passing around and manipulating Paths and never referring to paths as strings?

almostimplemented commented 1 year ago

I think typically, when a client passes a filename, they expect both string and pathlib.Path types to be valid input.

I can't speak with authority on best practice, but for example, numpy.load/save, torch.load/save, and mido.MidiFile all permit strings and Path objects.

It looks like torch has a type annotation of FILE_LIKE: TypeAlias = Union[str, os.PathLike, BinaryIO, IO[bytes]], and then checks:

def _is_path(name_or_buffer):
    return isinstance(name_or_buffer, (str, pathlib.Path))

The approach in numpy is to check if hasattr(file, 'read') # or: , 'write').

When the argument is explicitly a filename (as in the case of mido), you probably would not do this check and instead simply pass the value into open.

craffel commented 1 year ago

Thanks!