OpenCyphal / pydsdl

Cyphal DSDL processing front end implemented in Python
https://opencyphal.org
MIT License
10 stars 9 forks source link

Use of pathlib, os.path, and manual path transformation is brittle #84

Closed thirtytwobits closed 1 year ago

thirtytwobits commented 2 years ago

We mix our use of pathlib, os.path, and split/join manipulation of paths as strings and lists of strings. This causes brittleness when edge cases are encountered or when supporting different filesystems like Windows.

This task is to clean up all path handling to use pathlib consistently, use os.path only when specifically necessary, and to limit transformations between Path objects and strings.

Also to keep an eye on is the difference between PurePath and concrete paths. Use of Path.resolve or Path.absolute should be limited to concrete paths since the path names are first abstractions for the dsdl namespaces and are not, necessarily, compatible with canonical names in a filesystem. When in unittests it is important to distinguish between valid filenames and valid DSDL tree structures. For example, assert expected_file.samefile(actual_file) is correct regardless of canonical names whereas assert expected_path == actual_path is only true for pure, relative Path objects that have not been canonicalized.


Examples include:

os.path.join(
    os.path.split(self._root_namespace_path)[-1],
    os.path.relpath(self._file_path, self._root_namespace_path),
)

should be

self._root_namespace_path.name / self._file_path.relative_to(self._root_namespace_path)

this...

relative_directory, basename = [str(x) for x in os.path.split(relative_path)]

should be

relative_directory = str(relative_path.parent)
basename = str(relative_path.name)

In _test.py:

assert d.file_path == Path(_DIRECTORY.name, "uavcan", "test", "5000.Message.1.2.dsdl").resolve()

should be:

assert d.file_path.samefile(Path(_DIRECTORY.name, "uavcan", "test", "5000.Message.1.2.dsdl"))

etc