use pathlib in spiceypy for furnsh, etc

[AndrewAnnex]

feels like a good addition, but should be universal ie all file paths should be converted to pathlib Paths

[michaelaye]

I searched for path in spiceypy.py and only found that in the furnsh call()?
Are there any other file paths that would need a pathlib unwrap apart from furnsh?

[GregoireHENRY]

Actually every time you have a string that represents a path, e.g. when you open DAS/DSK files (dasopr, dskobj, unload).
Type for these strings could be a PATH: typing.Union[str, pathlib.Path]. To use them we could do str(PATH).

[michaelaye]

Sure, my question though was where a string "COULD" represent a path, as I was surprised to see only one function using paths but I see now where the confusion comes from: All other instances use other identifiers instead of path.

Here are the functions that need pathlib support:

• dashfn
• dasonw
• cklpf
• ckopn
• ldpool
• pcklof
• spklef
• spkopa
• spkopn
• unload
• cltext
• dafopr
• dafopw
• dasopr
• dasopw
• dskobj
• dskopn
• eklef
• ekopn
• ekopr
• ekopw
• exists
• fn2lun
• pckopn
• spkopn
• txtopn

Currently, these different identifiers are used for filepaths:

• name
• fname
• filename
• dsk
• path

Maybe these should be consolidated as well in a PR.

[GregoireHENRY]

This might be obvious but I put that down here. This change also affects tests, e.g. test_dasac_dasopr_dasec_dasdc in test_wrapper.py the assertion assert spice.dashfn(handle) == daspath line 1508 will fail if we only change file dashfn in spiceypy.py.
Because return type will be pathlib.Path and the assertion compares that and str.
Either we change it to assert str(spice.dashfn(handle)) == daspath or assert spice.dashfn(handle) == Path(daspath).

[GregoireHENRY]

made the changes and all 631 tests passed 🥳

[michaelaye]

I’m actually not sure if we want to return pathlib.Paths as that can potentially break a lot of people’s code. I was thinking as a first step we only ingest pathlib.Paths transparently, so that nobody would even notice a difference.

For returning pathlib.Paths I could imagine a config setting where the user could switch to either get strings or pathlib.Paths back with the default on strings? That way we could introduce returning pathlib without breaking anyone’s code.

@andrew what do you think?

[AndrewAnnex]

I had a busy day so I'm catching up here, but for sure it is important to not change any return types to Path to avoid a major version change just for this feature. That can always be decided later when another major release is issued, but I think it makes more sense to always accept strings or Paths, and internally use Pathlib to resolve relative paths before passing strings to cspice.

Also the 2nd point about different identifiers I strongly disagree with, although all the parameters are positional (for the most part) the names are consistent with the CSPICE docs. Changing the positional parameter names is not something I am willing to do. I am sure there may be counter examples in the code, but most likely those are typos from CSPICE that were corrected or typos in spiceypy itself.

Currently the PR #436 is not acceptable. Casting path's to strings naively negates the benefits of the Pathlib API. For this feature I was envisioning a new helper method within support_types that would attempt to resolve the absolute paths etc for the Path objects, then convert to strings, likely with some logging additions to make it clear why the user supplied path does not match the path the Spice error system reports if an error is raised. I may have an early attempt of this locally that I can dig up (or is it already pushed?), but it isn't as trivial as casting things to strings.

[michaelaye]

Considering that I so far have only seen one function (dasonw) where the path parameter name actually matches CSPICE, I don't think it's fair to say it's consistent as of yet.

Unless I'm looking somewhere wrong?
I am looking at https://naif.jpl.nasa.gov/pub/naif/toolkit_docs/C/cspice/index.html and comparing it with spiceypy.py function signatures.

A few examples of mismatches of the name for a path parameter, there seems to be a consistent replacement of fname and file with filename?

Function	CSPICE docs	spiceypy.py
furnsh	file	path
dskobj	dskfnm	dsk
cklpf	fname	filename
ckopn	fname	filename
spkopa	file	filename
spkopn	fname	filename
unload	file	filename

Do you want PRs to make it consistent? Or not worth to bother? Possibly the latter.

[GregoireHENRY]

Okay @AndrewAnnex & @michaelaye, your points make lot of sense.
I'll try a more complete type definition in the helper to benefit pathlib API.
Do you prefer to resolve path when an instance of this new type is created (in self.__init__()) or when str() is called (by overwritting self.__str__()) ?

About the logging additional info at error, it's a little bit unclear, SPICE will tell that the path does not exist and so?

[AndrewAnnex]

@michaelaye I'll need to think about that, I'd say it is better to be consistent so maybe in all places we could use filename or just correct each to match the cspice names more closely but it's a low priority for sure since they are all positional anyways

@GregoireHENRY no I am thinking a whole new function is necessary, along the lines of string_to_char_p. That function could use the calls like resolve expanduser and absolute to find the complete path. Overriding __str__ is really not what I would have in mind for this and I think doing this right is not going to be as trivial as casting to str. Users working with pathlib can already trivially cast to str themselves, but it would be nice to do some work in spiceypy to ensure the resolved filepaths can be used by cspice.

As for the logging I would need to think about it more, but potentially a new decorator could be written to keep track of what path the user provided and what path Pathlib found for the file, but I am not sure yet if that is the best solution at the moment.