-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fixes #13274: Allow cygwin-paths on Windows #13297
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
base: main
Are you sure you want to change the base?
Changes from 13 commits
382071f
93087c9
568bb34
2f87d70
0f5cce4
c343086
773a4eb
2334c9e
37c6203
19d92f3
e6aea3b
025a2eb
70cc464
249f38f
5241f3c
385ab81
2c40476
fde43a6
35a84ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
|
||
requires info.picocli; | ||
opens org.jabref.cli; | ||
opens org.jabref.cli.converter; | ||
|
||
requires transitive org.jspecify; | ||
requires java.prefs; | ||
|
@@ -20,9 +21,8 @@ | |
requires org.tinylog.api; | ||
requires org.tinylog.api.slf4j; | ||
requires org.tinylog.impl; | ||
|
||
requires java.xml; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please do not modify lines of code not related to the PR if not 90% sure Here, the empty line does make sence to separate blocks. Please undo this change. |
||
|
||
kvitorr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// region: other libraries (alphabetically) | ||
requires io.github.adr; | ||
// endregion | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
import java.io.File; | ||
|
||
import org.jabref.cli.converter.FilePathConverter; | ||
import org.jabref.logic.l10n.Localization; | ||
|
||
import static picocli.CommandLine.Command; | ||
|
@@ -15,10 +16,10 @@ class CheckIntegrity implements Runnable { | |
@Mixin | ||
private ArgumentProcessor.SharedOptions sharedOptions = new ArgumentProcessor.SharedOptions(); | ||
|
||
@Parameters(index = "0", description = "BibTeX file to check", arity = "0..1") | ||
@Parameters(index = "0", converter = FilePathConverter.class, description = "BibTeX file to check", arity = "0..1") | ||
private File inputFile; | ||
|
||
@Option(names = {"--input"}, description = "Input BibTeX file") | ||
@Option(names = {"--input"}, converter = FilePathConverter.class, description = "Input BibTeX file") | ||
private File inputOption; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change theese to Paths. Artifact from initial implementation. |
||
|
||
@Option(names = {"--output-format"}, description = "Output format: txt or csv") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,8 @@ | |
import javax.xml.parsers.ParserConfigurationException; | ||
import javax.xml.transform.TransformerException; | ||
|
||
import org.jabref.cli.converter.PathConverter; | ||
import org.jabref.cli.converter.StringPathConverter; | ||
import org.jabref.logic.exporter.Exporter; | ||
import org.jabref.logic.exporter.ExporterFactory; | ||
import org.jabref.logic.exporter.SaveException; | ||
|
@@ -36,13 +38,13 @@ public class Convert implements Runnable { | |
@Mixin | ||
private ArgumentProcessor.SharedOptions sharedOptions = new ArgumentProcessor.SharedOptions(); | ||
|
||
@Option(names = {"--input"}, description = "Input file", required = true) | ||
@Option(names = {"--input"}, converter = StringPathConverter.class, description = "Input file", required = true) | ||
koppor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
private String inputFile; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change this to Path |
||
|
||
@Option(names = {"--input-format"}, description = "Input format") | ||
private String inputFormat; | ||
|
||
@Option(names = {"--output"}, description = "Output file") | ||
@Option(names = {"--output"}, converter = PathConverter.class, description = "Output file") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The use of PathConverter for the output file path conversion is not optimal. Consider using Path.of() for better readability and maintainability. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strange that one os PathConverter and the other one is StringPathConverter. OK, this seems to be generated with AI. Please, instruct the AI to avoid code duplication.
While scrolling through, you should have seen this for yourself @kvitorr! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this could be artifacts from initial implementation. Should all be Paths. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @koppor yes, I am using AI to understand the code... I have never worked with picocli before... The converters were made taking into account the type of data that was returned by --input and --output options... So, if there was code duplication it was because there was no consensus on the return type of these variables. I didn't feel comfortable changing the type without knowing the consequences, so I uploaded the changes and waited for the code review. |
||
private Path outputFile; | ||
|
||
@Option(names = {"--output-format"}, description = "Output format") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
import java.nio.file.Path; | ||
import java.util.Optional; | ||
|
||
import org.jabref.cli.converter.StringPathConverter; | ||
import org.jabref.logic.importer.ParserResult; | ||
import org.jabref.logic.l10n.Localization; | ||
import org.jabref.logic.pseudonymization.Pseudonymization; | ||
|
@@ -34,10 +35,10 @@ public class Pseudonymize implements Runnable { | |
private ArgumentProcessor.SharedOptions sharedOptions = new ArgumentProcessor.SharedOptions(); | ||
|
||
@ADR(45) | ||
@Option(names = {"--input"}, description = "BibTeX file to be pseudonymized", required = true) | ||
@Option(names = {"--input"}, converter = StringPathConverter.class, description = "BibTeX file to be pseudonymized", required = true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The description should use 'BibTeX' as the spelling for BibTeX in Java strings, as per the guidelines. This ensures consistency across the codebase. |
||
private String inputFile; | ||
|
||
@Option(names = {"--output"}, description = "Output pseudo-bib file") | ||
@Option(names = {"--output"}, converter = StringPathConverter.class, description = "Output pseudo-bib file") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The description should use 'BibTeX' as the spelling for BibTeX in Java strings, as per the guidelines. This ensures consistency across the codebase. |
||
private String outputFile; | ||
|
||
@Option(names = {"--key"}, description = "Output pseudo-keys file") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
package org.jabref.cli.converter; | ||
|
||
import java.io.File; | ||
|
||
import org.jabref.logic.util.io.FileUtil; | ||
|
||
import picocli.CommandLine; | ||
|
||
/// Converts Cygwin-style paths to File objects using platform-specific formatting. | ||
public class FilePathConverter implements CommandLine.ITypeConverter<File> { | ||
|
||
@Override | ||
public File convert(String filePath) { | ||
String normalizedPath = FileUtil.convertCygwinPathToWindows(filePath); | ||
return new File(normalizedPath); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please do only use Path. If sthg needs to be converted, do it inline. File should represent an actual File. Path (nio) represents just the path. Unify the different Converter classes. This is going to be a maintenance hell later. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package org.jabref.cli.converter; | ||
|
||
import java.nio.file.Path; | ||
import java.nio.file.Paths; | ||
|
||
import org.jabref.logic.util.io.FileUtil; | ||
|
||
import picocli.CommandLine; | ||
|
||
/// Converts Cygwin-style paths to Path objects using platform-specific formatting. | ||
public class PathConverter implements CommandLine.ITypeConverter<Path> { | ||
|
||
@Override | ||
public Path convert(String value) { | ||
String normalizedPath = FileUtil.convertCygwinPathToWindows(value); | ||
return Paths.get(normalizedPath); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Path.of |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
package org.jabref.cli.converter; | ||
|
||
import org.jabref.logic.util.io.FileUtil; | ||
|
||
import picocli.CommandLine; | ||
|
||
/// Converts Cygwin-style paths to the standard format of the operating system. | ||
/// Especially useful on Windows to handle paths like /c/Users/... -> C:\Users\... | ||
public class StringPathConverter implements CommandLine.ITypeConverter<String> { | ||
|
||
@Override | ||
public String convert(String filePath) { | ||
return FileUtil.convertCygwinPathToWindows(filePath); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ | |
import org.jabref.logic.FilePreferences; | ||
import org.jabref.logic.citationkeypattern.BracketedPattern; | ||
import org.jabref.logic.layout.format.RemoveLatexCommandsFormatter; | ||
import org.jabref.logic.os.OS; | ||
import org.jabref.logic.util.StandardFileType; | ||
import org.jabref.model.database.BibDatabase; | ||
import org.jabref.model.database.BibDatabaseContext; | ||
|
@@ -589,4 +590,39 @@ public static String shortenFileName(String fileName, Integer maxLength) { | |
public static boolean isCharLegal(char c) { | ||
return Arrays.binarySearch(ILLEGAL_CHARS, c) < 0; | ||
} | ||
|
||
/// Converts a Cygwin-style file path to a Windows-style path if the operating system is Windows. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add something like "plainly converted if on non-Windows" |
||
/// | ||
/// Supported formats: | ||
/// - /cygdrive/c/Users/... → C:\Users\... | ||
/// - /mnt/c/Users/... → C:\Users\... | ||
/// - /c/Users/... → C:\Users\... | ||
/// | ||
/// @param filePath the input file path | ||
/// @return the converted path if running on Windows and path is in Cygwin format; otherwise, returns the original path | ||
public static String convertCygwinPathToWindows(String filePath) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. return Path object |
||
if (!OS.WINDOWS || filePath == null) { | ||
return filePath; | ||
} | ||
|
||
if (filePath.startsWith("/mnt/") && filePath.length() > 5) { | ||
String driveLetter = filePath.substring(5, 6).toUpperCase(); | ||
String path = filePath.substring(6).replace("/", "\\\\"); | ||
return driveLetter + ":" + path; | ||
} | ||
|
||
if (filePath.startsWith("/cygdrive/") && filePath.length() > 10) { | ||
String driveLetter = filePath.substring(10, 11).toUpperCase(); | ||
String path = filePath.substring(11).replace("/", "\\\\"); | ||
return driveLetter + ":" + path; | ||
} | ||
|
||
if (filePath.matches("^/[a-zA-Z]/.*")) { | ||
String driveLetter = filePath.substring(1, 2).toUpperCase(); | ||
String path = filePath.substring(2).replace("/", "\\\\"); | ||
return driveLetter + ":" + path; | ||
} | ||
|
||
return filePath; | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.