-
-
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?
Conversation
Created the method convertCygwinPathToWindows in the FileUtil class. It transforms a file path from Cygwin-style to Windows-style only if the operating system is Windows. The method follows this logic: 1. If the filePath is null or the OS is not Windows, return it unchanged. 2. If the path starts with /cygdrive/{drive} or /{drive}, replace it with {driveLetterUpperCase}:\ and convert forward slashes to backslashes. Based on research, the two common Cygwin-style formats are: - /cygdrive/{drive}/some/path - /c/some/path If none of theses patters match, or if the OS is not Windows, the original path is returned. Additionally, unit tests for this method were added to FileUtilTest class OBS: I need help to run the second test when Windows is disabled... I am getting an error because the test is not recognized...
…mportException I need help to understand if I really need to do this change... it was not clear to me on the issue description. (it is my first open source contribuition)
Do not know if I need to do that.
… into fix-for-issue-13274
Please add a CHANGELOG.md entry 😅 |
Tests look good! Now, weave it in into JabKit |
@@ -84,7 +84,7 @@ protected static Optional<ParserResult> importFile(String importArguments, | |||
LOGGER.debug("Importing file {}", importArguments); | |||
String[] data = importArguments.split(","); | |||
|
|||
String address = data[0]; | |||
String address = FileUtil.convertCygwinPathToWindows(data[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method 'convertCygwinPathToWindows' should be documented with non-trivial JavaDoc to explain its purpose and usage, as per instruction 23.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, this could be too late - isn't there a picocli transformation helper? Like that at the --input parameter is transformed on picocli level not at the first usage?
@koppor, can you help me? I’m not sure where to create the test for the ArgumentProcessor class. Also, would you like me to rename the test cases? |
No need to create tests there. Just implement the transformation of the --input (and --output) filename(s). |
already done that in the last commit |
See my comment. Is it possible to move the logic towards the --import argument? Reason: should also work at pseudonymization and check consistency, not only import. (If I understand the current diff right currently only at the import command it's working. Did you try out for pseudonymize?) |
sorry... I didn't see your comment. I'll try to understand what are you saying and then I'll comeback with some commit or comment |
Maybe it got lost while I was on the road. There is a place in the code where --input is mapped to a Java object variable by the picocli annotation @option (or similar). Maybe, there is a transformion possible. |
private String inputFile; | ||
|
||
@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 comment
The 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 comment
The 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.
PathConverter
is the nicer name, remove the other things.
While scrolling through, you should have seen this for yourself @kvitorr!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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 comment
The 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.
@@ -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 comment
The 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 comment
The 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 = {"--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 comment
The 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.
PathConverter
is the nicer name, remove the other things.
While scrolling through, you should have seen this for yourself @kvitorr!
@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 comment
The reason will be displayed to describe this comment to others. Learn more.
Change theese to Paths. Artifact from initial implementation.
private String inputFile; | ||
|
||
@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 comment
The 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.
@@ -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) | |||
private String inputFile; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this to 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Path.of
/// | ||
/// @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 comment
The reason will be displayed to describe this comment to others. Learn more.
return Path object
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 comment
The 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.
Please put in your classname sthg about Cygwin Path, because picocli does nativly support Path as a filetype ( https://picocli.info/quick-guide.html#_type_conversion ) and nobody gets the impression that this is some unneccessary conversion class. |
You have to do your changes (add line, remove line) manually. GitHub does not yet support nativly commands in the comment section to modify your code. Please always remember: https://github.com/JabRef/jabref/blob/main/CONTRIBUTING.md#notes-on-ai-usage |
@calixtus thank you for your time, I will do the changes and then wait for the feedback. Please be patient xD, I am doing my best. |
…s that represents the cygwin-paths known and the /c/ pattern
… into fix-for-issue-13274
@trag-bot didn't find any issues in the code! ✅✨ |
I made new changes to the project...
Other than that, I didn't manually test the changes but I wanted to upload the modification right away so you could give me feedback on whether I understood the requested changes correctly. I also updated the pull request description. Regarding the use of AI, I understand your point of view, but I didn't find your comments interesting. I use it to understand the code, understand dependencies, understand business rules, make comparisons between technologies, understand differences between data types, and try to bring more context to some of the comments you make. The pull request description was not made using AI, and I understood the changes that were made and also tested it locally. Yes, there was code duplication, but I didn't understand whether the data type received by the CLI could be changed from String/File to Path. My thought at the time was "if the code is using different data types for input and output, there must be a reason." And that's why I created the three converters, String, File, and Path. So I thought, I'll upload the change and wait for the review. If I need to change something, it will be requested. Since this is my FIRST open source contribution, I expected it to be a cool experience. In fact, I'm learning a lot, having contact with organized code, with tests, with documentation and with Picocli, but these comments about the use of AI caught me by surprise. I read Daniel Roe's text (https://roe.dev/blog/using-ai-in-open-source) and I feel like I didn't deviate from anything he said. But since this was pointed out to me, I also want to point out something to you, that you at least read the entire description of the pull request and answer any questions written there... this will help clarify ideas, allow a better understanding of the issue and maintain a good environment for both of us. And, as it is said in the contribution guide, you can reject the PR if you think it doesn't contribute anything to this issue. |
Sorry you got offended by our comment about ai. It's partially a precaution since we get a lot of contributions lately that are completely ai generated, look nice on first sight, but are rubbish on second. We are not against use of ai, in fact we use it ourselves for repeating task, for simple refactors etc. The general rule is: never to use generated stuff without reading it, testing it and owning it. In short: Never let an ai think for you. Never let an ai speak for you. There were some indicators, that some of your changes were generated. Maybe it is also our perspective, that makes look changes a newcomer made without much experience so partially similar to changes a LLM does. Eg. Your comments remove line and add line looked like commands one wanted to send to an ai. Please don't be alienated by our precautions. We're always happy about newcomers and try to be welcoming. This is why we did comment on your changes and not just close the pr without comment because we would have thought that this would just be another ai pr. |
Looks good to me so far... |
/// @return the converted path if running on Windows and path is in Cygwin format; otherwise, returns the original path | ||
public static Path convertCygwinPathToWindows(String filePath) { | ||
if (filePath == null) { | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New public methods should not return null. The method should return Optional instead to better handle the null case and follow Java best practices.
return Path.of(filePath); | ||
} | ||
|
||
/// Builds a Windows-style path from a Cygwin-style path using a known prefix index. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Triple-slash comments are used instead of proper JavaDoc format. Additionally, the comment is trivial and doesn't add new information beyond what the code shows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comments.
Please also have a look at the failing jbang tests - maybe, some interface changed and needs to be adapted?
https://github.com/JabRef/jabref/actions/runs/15646674264/job/44085291757?pr=13297
@@ -0,0 +1,16 @@ | |||
package org.jabref.cli.converter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For naming, I think, this should be with "s", because there will be more than one converters in the future.
pacakge org.jabref.cli.converters
.
@@ -22,7 +23,6 @@ | |||
requires org.tinylog.impl; | |||
|
|||
requires java.xml; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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.
/// | ||
/// @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 Path convertCygwinPathToWindows(String filePath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method also works on linux - proposal for another name:
public static Path convertCygwinPathToWindows(String filePath) { | |
public static Path convertStringToPathRespectingCygwin(String filePath) { |
@@ -589,4 +593,47 @@ 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Add something like "plainly converted if on non-Windows"
Closes #13274
Allow cygwin-paths on Windows
Create the method convertCygwinPathToWindows in the FileUtil class. It transforms a file path from Cygwin-style to Windows-style only if the operating system is Windows.
Based on research, the two common Cygwin-style formats are:
The third was asked to be added
The method follows this logic:
Additionally, unit tests for this method were added to FileUtilTest class
OBS: I need help to run the second test when Windows is disabled... I am getting an error because the test is not recognized...
It was created a Converter, called CygwinPathConverter, as requested on the code review... and it was added to the Option Anottation in the classes: CheckConsistency.java, CheckIntegrity.java, Convert.java, Pseudonymize.java, Search.java
Example of use:
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)