Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use filepath instead of path #188

Merged
merged 1 commit into from
Oct 13, 2022
Merged

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Oct 10, 2022

Originally, this package was documented to be for paths separated by forward slashes, but I was recently pointed to the updated package description, which amended the documentation (see https://go-review.googlesource.com/32423) and added:

To manipulate operating system paths, use the path/filepath package.

A later patch (https://go-review.googlesource.com/45653) tweaked the description again, adding to the path/filepath package:

To process paths such as URLs that always use forward slashes regardless of
the operating system, see the path package.

And to the path package

This package does not deal with Windows paths with drive letters or backslashes;
to manipulate operating system paths, use the path/filepath package.

I expect it to be "fine" in general to use the path package for Unix/Linux, but
from both patches, the intent of the Go maintainers is to use path/filepath
for anything file(system) related, so let's adapt to that.

Originally, this package was documented to be for paths separated by forward
slashes, but I was recently pointed to the updated package description, which
amended the documentation (see https://go-review.googlesource.com/32423) and
added:

> To manipulate operating system paths, use the path/filepath package.

A later patch (https://go-review.googlesource.com/45653) tweaked the description
again, adding to the `path/filepath` package:

> To process paths such as URLs that always use forward slashes regardless of
> the operating system, see the path package.

And to the `path` package

> This package does not deal with Windows paths with drive letters or backslashes;
> to manipulate operating system paths, use the path/filepath package.

I expect it to be "fine" in general to use the `path` package for Unix/Linux, but
from both patches, the intent of the Go maintainers is to use `path/filepath`
for anything file(system) related, so let's adapt to that.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@rhatdan
Copy link
Collaborator

rhatdan commented Oct 12, 2022

LGTM
@kolyshkin @giuseppe @lsm5 PTAL

Copy link
Collaborator

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am slightly more inclined towards using path rather than filepath, as path is somewhat simpler, and will definitely work fine on Linux (and this code is Linux-specific and will stay that way).

OTOH I don't see a reason to not use filepath, so LGTM.

@rhatdan rhatdan merged commit bb87a32 into opencontainers:main Oct 13, 2022
@thaJeztah thaJeztah deleted the use_path branch October 13, 2022 22:07
@thaJeztah
Copy link
Member Author

Yup; I originally thought "path" was the way to go as well. Fun fact (but I need to give it another go), was that I did some benchmarking at some time, and filepath was more performant than path (perhaps they added some optimisation that they didn't add to path ?

Also if their intent is for filepath to be used for file paths, then there is "some" risk that path may be missing some (obscure?) handling for file-specifics, or at least their focus may not be on that, so using filepath would be the safer option I guess?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants