feat: make PebbleClient file methods also accept pathlib.PurePath

[james-garner-canonical]

I got tripped up while working on #2087 with methods like ops.pebble.PebbleClient.push only accepting a str for their path argument, while the wrapper methods like ops.Container.push accept str | pathlib.PurePath. Nowadays pretty much everything you're likely to run into in the stdlib that operates on file paths will accept path-like objects, so it seems nice to have the same consistency here.

• This PR updates the PebbleClient methods to also accept pathlib.PurePath, immediately converting the argument to str to avoid the signature change affecting their implementation.
• The equivalent methods on _TestingPebbleClient have been updated the same way.
• The ops.Container calls to these methods have been updated to not convert the path argument to str since it will be converted after calling anyway.

[benhoyt]

Looks reasonable to me, thanks. We should probably have unit tests that exercise this with a pathlib.Path instance, at least for the pebble.py methods.

[benhoyt]

I think this type and implementation makes sense given the existing types.

However, I'm just curious: for new code, do you know if using the type os.PathLike and path = os.fspath(path) be the more general approach?

[james-garner-canonical]

Yes, that would be the 'right' way to do it, though annoyingly os.PathLike is generic because __fspath__ can return str or bytes, so we'd probably want to use os.PathLike[str].

Also for new code I'd tend towards using path = pathlib.Path(path) internally where possible.

[tonyandrewmeyer]

Also for new code I'd tend towards using path = pathlib.Path(path) internally where possible.

In that case, would it not be better to make that change here too? It's more invasive, but I doubt significantly so, and if it's the better choice, wouldn't we rather do that now? It seems unlikely it would happen in the future.

[james-garner-canonical]

For these pebble methods specifically, I don't think there's a benefit in working with Path objects for the arguments, since they're typically immediately just used as values in a dict for a request.

[james-garner-canonical]

We should probably have unit tests that exercise this with a pathlib.Path instance, at least for the pebble.py methods.

EDIT: I guess this opening paragraph is wrong, since the test_pebble tests which skip the Harness backend use str arguments.

These actually get exercised (as much as they were previously with strings) with paths already now, due to how the change is implemented. Previously the ops.Container methods were tested with pathlib.Paths, which they converted into strings and passed to the pebble methods. Now they pass them without converting them, since they're converted internally.

However, the current tests exercising this seem to be Harness based, so they're using Harness's mock pebble client instead of the real one.

Would it be worth perhaps parametrizing (or just duplicating) a number of the test_pebble tests to use pathlib.Path arguments instead of str?

[benhoyt]

Ah, in that case, the tests we have already is probably fine.

[tonyandrewmeyer]

I do think we should have some new tests here (I would guess there would be existing pebble ones that could just be parametrized). Partly because of your edit to the earlier comment, but mostly because I feel that since pebble.py can be used as a standalone Python Pebble API module, it shouldn't rely on Ops tests for test coverage.

For consistency, do you think exec's working_dir should also have this change?

[tonyandrewmeyer]

Also for new code I'd tend towards using path = pathlib.Path(path) internally where possible.

In that case, would it not be better to make that change here too? It's more invasive, but I doubt significantly so, and if it's the better choice, wouldn't we rather do that now? It seems unlikely it would happen in the future.

[james-garner-canonical]

For consistency, do you think exec's working_dir should also have this change?

This would be more consistent overall. The reason I didn't cover this parameter is because ops.Container.exec doesn't accept a Path here, only str | None.

I think it would be fine to merge this PR without adding this additional feature. However, since we won't be merging without adding additional tests, it shouldn't be much work to also add Path support to working_dir, so I'll do that.

working_dir being None-able makes me think we should use os.fspath instead of str for conversion, since that will ensure we handle None correctly (instead of converting it to 'None').

[james-garner-canonical]

I've opted not to bundle switching from str to os.fspath with this PR.

It's technically not backwards compatible.

Previously if someone used None or some other strange object as the path argument to an ops.Container method, it would get stringified and probably just work, writing to some strangely named file. It would also work (though it probably shouldn't have) with pathops.ContainerPath objects, which have a path-like str, but are intentionally not os.PathLike.

If we want to switch, I think it should be a separate PR, so it can be more easily reverted if it causes trouble.

[tonyandrewmeyer]

Thanks for adding the tests! Looks good to me.

[dimaqq]

LGTM