Add EndNote XML importer #3713
Add EndNote XML importer #3713
Conversation
tobiasdiez
added
the
status: ready-for-review
|
@Siedlerchr Can you please try to import the test file in Linux. The user reports that JabRef freezes after he confirms the import dialog but I can't reproduce the problem here on Windows. Thanks. |
|
Xubuntu 16.04 works fine. No issues. |
|
The problem is that in the xml file from the user there are records in which apparently many fields are null and thus result in an NPE. The root problem I see here is that this thing like Titles etc all are serialized to their own object and not to collections and some of them might be null. |
|
I think I found an idea: |
| .getRecord().stream() | ||
| .map(this::parseRecord) | ||
| .collect(Collectors.toList()); | ||
| .getRecord() |
There was a problem hiding this comment.
@Siedlerchr Thanks for the fix! However, it appears there is still a problem with your eclipse style configuration (alignment at dot).
| public static void testImportEntries(Importer importer, String fileName, String fileType) throws IOException, ImportException { | ||
| ParserResult parserResult = importer.importDatabase(getPath(fileName), StandardCharsets.UTF_8); | ||
| if (parserResult.isInvalid()) { | ||
| throw new ImportException(parserResult.getErrorMessage()); |
There was a problem hiding this comment.
Why not throw an IOException instead? Or maybe put the call to importDatabase into a try-catch that converts the IOException into an ImportException?
Overall, it would be better to throw just one Exception type. Then you don't have to change all tests to throws Exception, which imho is ugly, because it is so generic.
There was a problem hiding this comment.
Isn't the generic Exception the preferred choice for tests anyway? It has the advantage that you not need to change your tests if you decide to change the exception signature of the method under test.
I prefer to keep it that way. The error is not coming directly from the file system and thus it is not an IOException in my opinion.
There was a problem hiding this comment.
In tests it doesn't matter which exceptions are thrown
| Objects.requireNonNull(reader); | ||
|
|
||
| try { | ||
| JAXBContext context = JAXBContext.newInstance("org.jabref.logic.importer.fileformat.endnote"); |
There was a problem hiding this comment.
Constructing a JAXBContext is very expensive. It is so expensive that you notice it even as a human. Please turn this into an instance variable, then the construction takes place only once per importer object. Or you could just store the Unmarshaller, which should be enough as well.
There was a problem hiding this comment.
Good to know! I actually copied this part from the medline importer and will change it there too.
| public boolean isRecognizedFormat(BufferedReader reader) throws IOException { | ||
| String str; | ||
| int i = 0; | ||
| while (((str = reader.readLine()) != null) && (i < 50)) { |
There was a problem hiding this comment.
That's a funny way of determining the format :-) Somewhere in the first 50 lines a records tag has to appear? Why up to 50? What if some random garbage comes before the records tag? Would JAXB still be able to parse that?
There was a problem hiding this comment.
That's another thing I just copied from the medline importer. Can jaxb test the file without actually parsing everything? If not I find the "record appears near the beginning" a relative good heuristic.
There was a problem hiding this comment.
Nope, JAXB will try to parse everything. So you could go with such a heuristic, but could you just improve the code a bit? E.g. no side effect (line reading) in the condition of the while, promoting the magic number 50 to a private final static and so on.
|
Coday is just complaining about the juntit 5 annotations, but that's not your fault. Otherwise from my side code is good as well. |
As wished in the forum.