Fix path conversion in JabKit pseudonymization#14125
Conversation
Hey @ravatex!Thank you for contributing to JabRef! Your help is truly appreciated ❤️. We have automatic checks in place, based on which you will soon get automated feedback if any of them are failing. We also use TragBot with custom rules that scans your changes and provides some preliminary comments, before a maintainer takes a look. TragBot is still learning, and may not always be accurate. In the "Files changed" tab, you can go through its comments and just click on "Resolve conversation" if you are sure that it is incorrect, or comment on the conversation if you are doubtful. Please re-check our contribution guide in case of any other doubts related to our contribution workflow. |
subhramit
left a comment
There was a problem hiding this comment.
Small possibility for more improvement.
When you go with this, also check if any test needs to be added/adapted for it.
| private Path resolveOutputPath(String customPath, Path inputPath, String defaultFileName) { | ||
| return customPath != null ? Path.of(customPath) : inputPath.getParent().resolve(defaultFileName); | ||
| return customPath != null ? Path.of(customPath) : inputPath.toAbsolutePath().getParent().resolve(defaultFileName); | ||
| } | ||
|
|
||
| private Path resolveOutputPath(Path customPath, Path inputPath, String defaultFileName) { | ||
| return customPath != null ? customPath : inputPath.toAbsolutePath().getParent().resolve(defaultFileName); | ||
| } |
There was a problem hiding this comment.
Just an idea - I think now the keys file can also be input as a Path (by using Cygwin paths) instead of a String, and in that case the method below would be sufficient for both and the one above can be removed?
|
I assumed there was a reason for originally having it as a string instead of a path. I can fix this. Edit: There were no tests for this class, as it is just an argument interface for the |
|
|
You can take a look at https://github.com/JabRef/jabref/blob/main/jabkit/src/test/java/org/jabref/cli/ArgumentProcessorTest.java |
|
Should I create a new issue for creating the rest of the CLI tests. I don't think it should be in this PR since there are opportunities for abstractions for nice CLI tests, adding it here seems not clean. |
Yes, that sounds good. We can consider this PR done in the current state. |
* fix: corrected path conversion logic * refactored keyFile to be Path * fixed checkstyle violation
Closes #14122
The infrastructure for figuring out a reasonable default path was already there, the logic was just a little bit off.
Now the
outputandkeyarguments should be actually optional.Test it via
$ jabkit pseudonymize --input any.bibI was not able to find any automated CLI end to end testing I can implement. If there is a method inform me to add to this PR.
Mandatory checks
CHANGELOG.mdin a way that is understandable for the average user (if change is visible to the user)