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 Delombok option to keep full path within output directory #3343

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

Conversation

tamasvajk
Copy link

This PR introduces a new --target-keep-path flag for delombok. It should be used in combination with --target. The main goal is that individual files passed to delombok could be written to the output directory without accidentally overwriting each other.

As an example:

java -jar lombok.jar delombok X/A.java X/B.java Y/A.java -d out

produces two files in the out folder, A.java and B.java. Note that A.java that's processed earlier will be overwritten by the other file named the same.

On the other hand:

java -jar lombok.jar delombok X/A.java X/B.java Y/A.java -d out --target-keep-path

produces out/[FULL_PATH_PREFIX]/X/A.java, out/[FULL_PATH_PREFIX]/X/B.java, out/[FULL_PATH_PREFIX]/Y/A.java, where [FULL_PATH_PREFIX] is the full path of the parent folder of X and Y.

Comment on lines 916 to 927
private static String normalizeWindowsPath(String path)
{
/* Change `c:\aa\Bb\cc` path to `C/aa/Bb/cc`. */
if (path.length() >= 2 && path.charAt(1) == ':') {
char driveLetter = path.charAt(0);
if (driveLetter >= 'a' && driveLetter <= 'z') {
path = Character.toUpperCase(driveLetter) + path.substring(2);
}
}
path = path.replace('\\', '/');
return path;
}
Copy link
Author

Choose a reason for hiding this comment

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

I haven't tested this on Windows. I think the minimum that we would need to do is remove the :, which is not valid in a path outside of the drive path. Changing \ to / might not be necessary. I think the path is case sensitive in Windows, but not the drive letter, so I upper-cased it, but this might also not be required. I'd appreciate some input from people on Windows.

Copy link

Choose a reason for hiding this comment

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

neither the drive letter nor the path are case sensitive on windows, unless you might change some obscure setting in Windows 11, or something. So I wouldn't change the capitalization.

Removing the colon ˋ:ˋ though generates a problem: it needs to come after the drive letter! Otherwise the path is relative to the current drive.

As I don't see how it's used here there's an exception though: if a path gets converted to a url the colon has to go away and it would actually look similar to this source code. Reasoning: colon is a delimiter between protocol and path to a resource in a url. So it would look like file:///C/path/to/file.
But windows-style for the same path would be C:\path\to\file, or alternatively C:/path/to/file.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for confirming, I'll remove the case changing.

Regarding the colon removal: This method is used in outFile = new File(outBase, normalizeWindowsPath(file.getPath()));, so the drive letter is actually becoming a folder name inside outBase. So removing the : seems to be needed.

Copy link

@dstango dstango Feb 3, 2023

Choose a reason for hiding this comment

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

the case thing made me curious, so I went on some research on the internet and found the following: https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#:~:text=see%20CreateFile.-,Volume%20designators,-(drive%20letters)%20are -- it has a bunch of more details.

While the drive letter isn't case sensitive, quite a bunch of other projects and websites talk of a "canonical form", in which the drive letter would be upper case. So the original capitalization wouldn't be bad.

Looking over the source code there's another detail in the implementation I'd like to point out: a path on windows can be relative, yet specify a drive: e.g. C:this\is\a\path could mean all sorts of paths, depending on the current directory on drive C:. If my current directory on C: is \temp, then the before mentioned path would reference the absolute path C:\temp\this\is\a\path. And: each drive can have a separate current directory ...

As I'm not aware of the whole context of the usage, generating a normalized path C/this/is/a/path (without the temp part) might be either desirable, albeit somewhat confusing. I personally would rather have the drive letter stripped completely in this case. Yet it might render the intention of this PR useless, as it's about keeping the full path ...

As you mention the usage will be in relation to file.getPath() I'm wondering if using file.getCanonicalPath() wouldn't be more appropriate. https://www.baeldung.com/java-path has some nice examples.

Copy link
Author

Choose a reason for hiding this comment

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

I've simplified this logic to file.getPath().replace(":", ""). I think this matches the intent of the PR: whatever path is added as an input, it will be used to create the filepath of the output. There's one adjustment for the : to handle the full path cases on windows, which would become invalid folder names otherwise.

I think this would result in overwritten files only if javac returned multiple JCCompilationUnits with source files having the same URIs.

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