Fix PDF relativization test#12621
Conversation
|
Blocked by #12619 |
There was a problem hiding this comment.
PR Overview
This PR fixes a failing test for PDF metadata file path relativization. The changes include:
- Modifying the test to use a manually instantiated FilePreferences rather than a mocked one.
- Adding an overloaded importDatabase method in PdfMergeMetadataImporter that relativizes the file path.
- Introducing a corresponding overloaded importDatabase method in PdfImporter to support the new file path handling.
Reviewed Changes
| File | Description |
|---|---|
| src/test/java/org/jabref/logic/importer/fileformat/pdf/PdfMergeMetadataImporterTest.java | Updates test to use manual file preferences instantiation for correct working directory setup |
| src/main/java/org/jabref/logic/importer/fileformat/PdfMergeMetadataImporter.java | Introduces a new method that relativizes the file path before database import |
| src/main/java/org/jabref/logic/importer/fileformat/pdf/PdfImporter.java | Adds an overloaded method accepting FilePreferences to resolve the file path correctly |
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| } | ||
| } | ||
|
|
||
| public ParserResult importDatabase(Path filePath, FilePreferences filePreferences) { |
There was a problem hiding this comment.
Not sure why this is necessary. Shouldn't the caller take care about the absolute path? - Since this method is a) a duplicate of public ParserResult importDatabase(Path filePath) and b) used only once, I propose to move Path readPath = filePreferences.getWorkingDirectory().resolve(filePath); to the caller.
There was a problem hiding this comment.
Yes... the problem is:
XmpUtilReader can't read relativized path. This was the reason why I (mistakenly) removed relativization.
So in this method I leave relativized path in BibEntry, but when I'm reading I use the full path.
Yeah, something's wrong here
koppor
left a comment
There was a problem hiding this comment.
Ah, let's go this through. Better than nothing. :)
I have a small suggestion, which would make the logic more clear.
Maybe, also the JavaDoc should be copied into PdfImporter.
| List<Path> directories = context.getFileDirectories(filePreferences); | ||
|
|
||
| filePath = FileUtil.relativize(filePath, directories); |
There was a problem hiding this comment.
This should be moved down to ´importDatabase`, then I understand it more...
There was a problem hiding this comment.
It is in importDatabase?
|
I should think about it more |
The quick fix is OK for now, isn't it? |
| } catch (IOException e) { | ||
| return ParserResult.fromError(e); | ||
| } | ||
| return new PdfMergeMetadataImporter(importFormatPreferences).importDatabase(file, context, filePreferences); |
There was a problem hiding this comment.
The try-catch block removal shifts exception handling responsibility to the caller without documenting this change in method signature. Method should declare throws IOException.
There was a problem hiding this comment.
IntelliJ said that it is not thrown at all
|
|
||
| public List<BibEntry> importDatabase(Path filePath, PDDocument document) throws IOException, ParseException { | ||
| return grobidService.processPDF(filePath, importFormatPreferences); | ||
| public List<BibEntry> importDatabase(Path fullPath, PDDocument document) throws IOException, ParseException { |
There was a problem hiding this comment.
The method accepts PDDocument parameter but doesn't use it in the implementation, violating the Single Responsibility Principle by having unused parameters.
There was a problem hiding this comment.
Yes, it just uses grobid.
Is there any way to mark this argument as explicitly unused?
There was a problem hiding this comment.
Why can't it be removed - I don't see any @Override annotation.
There was a problem hiding this comment.
Oh! It should be overriden
There was a problem hiding this comment.
Then you can add JavaDoc and use @param and then say that it is ignored ^^
|
If you made changes that are visible to the user, please add a brief description along with the issue number to the |
|
|
||
| public List<BibEntry> importDatabase(Path filePath, PDDocument document) throws IOException { | ||
| return new XmpUtilReader().readXmp(filePath, xmpPreferences); | ||
| public List<BibEntry> importDatabase(Path fullPath, PDDocument document) throws IOException { |
There was a problem hiding this comment.
Method accepts PDDocument parameter but doesn't use it in the implementation, violating Single Responsibility Principle by having unused parameters.
There was a problem hiding this comment.
Yes, this is expected.
Is there any way to mark this argument as explicitly unused?
Fixes failing test:
PdfMergeMetadataImporterTest#importRelativizesFilePath.Fixes #12620
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if change is visible to the user)