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

Add function to normalize path between different OS #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Azurelol
Copy link
Collaborator

@Azurelol Azurelol commented Jun 21, 2022

Description

Adds the normalizePath function on PropertyUtils, meant to replace IntegrationSpec.osPath in the commons-test library.

For example, given the path /foo/bar, on the following systems:

  • Windows: c:/foo/bar
  • Macos: /foo/bar

Changes

  • ADD PropertyUtils : normalizePath

@Azurelol Azurelol force-pushed the add/property_utils_normalize_path branch 2 times, most recently from 9daebfd to 8379165 Compare June 21, 2022 13:10
@Azurelol Azurelol marked this pull request as ready for review June 22, 2022 12:34
@Azurelol Azurelol requested a review from Larusso as a code owner June 22, 2022 12:34
Comment on lines +85 to +105
static String normalizePath(String path, String rootDirectory) {

Boolean absolute = path.startsWith('/')
if (absolute) {
if (windows) {
String volume = getDiskVolume()
path = "${volume}${path[1..-1]}"
}
}
else if (rootDirectory != null && rootDirectory != "") {
path = "${rootDirectory}/${path}"
}
new File(path).path
}

static String normalizePath(String path) {
normalizePath(path, null)
}
Copy link
Member

Choose a reason for hiding this comment

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

hmm I'm not quite sure how this should be used. If I have a relative path you will resolve it based on the root path given. Java has all these methods build in with the java.io.File Api

public String getAbsolutePath()
Returns the absolute pathname string of this abstract pathname.
If this abstract pathname is already absolute, then the pathname string is simply returned as if by the getPath() method. If this abstract pathname is the empty abstract pathname then the pathname string of the current user directory, which is named by the system property user.dir, is returned. Otherwise this pathname is resolved in a system-dependent way. On UNIX systems, a relative pathname is made absolute by resolving it against the current user directory. On Microsoft Windows systems, a relative pathname is made absolute by resolving it against the current directory of the drive named by the pathname, if any; if not, it is resolved against the current user directory.

Returns:
The absolute pathname string denoting the same file or directory as this abstract pathname
Throws:
SecurityException - If a required system property value cannot be accessed.
See Also:
isAbsolute()

I think the functionality you are describing in combination with a root path is resolve path

Path resolve(Path other)
Resolve the given path against this path.
If the other parameter is an absolute path then this method trivially returns other. If other is an empty path then this method trivially returns this path. Otherwise this method considers this path to be a directory and resolves the given path against this path. In the simplest case, the given path does not have a root component, in which case this method joins the given path to this path and returns a resulting path that ends with the given path. Where the given path has a root component then resolution is highly implementation dependent and therefore unspecified.

Parameters:
other - the path to resolve against this path
Returns:
the resulting path
See Also:
relativize

We need to be carful what a single helper method can do under the hood and what the expectations are.

For instance I think it would fail if the method is provided with these parameters on windows:

normalizePath("/root/path", "c:/another/root" -> c:/another/root/c:/root/path

a root directory to start from if the path is found to be relative

There is no early return if the path is absolute.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There not need be an early return in this case.

Copy link
Collaborator Author

@Azurelol Azurelol Jun 28, 2022

Choose a reason for hiding this comment

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

I am a bit confused on you mentioning you are not sure how this method could be used, as its meant to be the successor to osPath, which we liberally use throughout our tests. I just want to write in a more robust way.

Copy link
Member

Choose a reason for hiding this comment

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

You added functionality that was not in the old method. So if it is a successor I wonder where the resolve parts are used.
I see the else if must have overlooked it. But I personally would not mix to many things into one wonder function. It doesn't compose well. I would simply like to see where you need at the moment the functionality to resolve relative paths with a root directory because as I showed you there are builtin methods for that.

Also you don't validate the root directory. You take it as face value that this could be either in the correct format or not. At the very least you should call normalize again on that path to make sure it is in the correct format for the current platform.

@Azurelol Azurelol requested a review from Larusso June 24, 2022 08:56
@Azurelol Azurelol force-pushed the add/property_utils_normalize_path branch from 8379165 to 960cf49 Compare July 6, 2022 14:24
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.

2 participants