py_mesa_reader icon indicating copy to clipboard operation
py_mesa_reader copied to clipboard

Use pathlib library to handle filenames

Open sybreton opened this issue 1 year ago • 5 comments

The filename that is read by the read_data function currently assumes that self.file_name is a string and checks the file suffix with the string method endswith. However, this prevents from passing to the function a Path object created e.g. with the pathlib library, which might prove problematic when integrating the mesa_reader in other module test procedure, where the path to the profile to read cannot be anything else than a Path object.

The fix uses the pathlib library in order to check the suffix of self.file_name (in read_data) with the pathlib library suffix attribute.

sybreton avatar Oct 10 '24 07:10 sybreton

I'm not totally opposed to making this compatible with pathlib, but importing two new modules in adds some complexity to a rather simple tool. I wonder if we could just cast self.file_name to a string before using plain old endswith. The code would still be agnostic to whether the file name is a string or a path object. Do you see any likely issues with that?

wmwolf avatar Oct 17 '24 17:10 wmwolf

In my opinion, it's much better to use pathlib rather than plain strings. It's precisely the reason this library exists. It's a lot easier to manipulate and inspect Path objects than plain strings. I would argue that self.file_name and any other file related properties and variables should be stored as Path objects instead of a strings to begin with. It would make it much easier to handle both for us and the users. A Path object is compatible almost everywhere a string representing a path is required, such as when using open and even os.path functions (they can accept Path objects, but will still return strings). As a result, most users won't even notice if they get a string or a Path object, and if they really want a string, they can just use str(path).

ShaiAvr avatar Oct 18 '24 07:10 ShaiAvr

I totally agree with the point raised by @ShaiAvr.

@wmwolf, about your concern related to importation I do not see this as a particular issue in this case as pathlib is part of the Python standard library, so it does not require any additional installation.

(you mention the importation of two new modules, I do not really understand why, actually the only new module is pathlib)

sybreton avatar Oct 18 '24 07:10 sybreton

Sorry for going silent on this; the semester ending is always hectic. Given that it seems I have fallen behind the times on the standard library, I agree that this is a better approach. I will try to make sure that there are no unintended surprises that would be caused by this cahnge, but then I will issue a new release soon with pathlib as the backend. Thanks for your contribution, @sybreton!

wmwolf avatar Jan 15 '25 18:01 wmwolf

Perfect, thanks a lot !

sybreton avatar Jan 17 '25 14:01 sybreton