Add EndNote XML importer by tobiasdiez · Pull Request #3713 · JabRef/jabref

tobiasdiez · 2018-02-11T16:24:10Z

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)

tobiasdiez · 2018-02-12T09:28:03Z

@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.

Siedlerchr · 2018-02-12T09:40:45Z

Xubuntu 16.04 works fine. No issues.

Siedlerchr · 2018-02-12T20:56:08Z

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.
I fixed some of them.

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.
If it were mapped to collections we could just initalize it with empty collections and avoid the null shit.
Maybe this approach helps:
http://blog.bdoughan.com/2012/11/creating-generic-list-wrapper-in-jaxb.html

Siedlerchr · 2018-02-12T21:15:00Z

I think I found an idea:
First we generate an XSD file from a concrete xml file and then use that as input for xjc. This should hopefully be able to generate lists from what I read.. Or we convert the dtd to xsd first.

tobiasdiez · 2018-02-12T22:22:26Z

-                                                .getRecord().stream()
-                                                .map(this::parseRecord)
-                                                .collect(Collectors.toList());
+                        .getRecord()


@Siedlerchr Thanks for the fix! However, it appears there is still a problem with your eclipse style configuration (alignment at dot).

lenhard

I just have a few comments regarding the code. Not tested it in a running instance.

lenhard · 2018-02-13T14:07:56Z

+    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());


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.

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.

In tests it doesn't matter which exceptions are thrown

lenhard · 2018-02-13T14:11:42Z

+        Objects.requireNonNull(reader);
+
+        try {
+            JAXBContext context = JAXBContext.newInstance("org.jabref.logic.importer.fileformat.endnote");


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.

Good to know! I actually copied this part from the medline importer and will change it there too.

lenhard · 2018-02-13T14:14:20Z

+    public boolean isRecognizedFormat(BufferedReader reader) throws IOException {
+        String str;
+        int i = 0;
+        while (((str = reader.readLine()) != null) && (i < 50)) {


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?

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.

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.

…Bookends

…o importBookends

lenhard

Code-wise I have nothing to add.

I would say that if the user from the forum has tested the functionality and is happy with it, you can merge.

Siedlerchr · 2018-02-16T15:05:12Z

Coday is just complaining about the juntit 5 annotations, but that's not your fault. Otherwise from my side code is good as well.

Add EndNote XML importer

37a3072

tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 11, 2018

add some optionals of Nullable to fix parsing errors

0930460

tobiasdiez commented Feb 12, 2018

View reviewed changes

lenhard reviewed Feb 13, 2018

View reviewed changes

tobiasdiez added 5 commits February 14, 2018 15:26

Implement feedback

fee07d4

Merge branch 'master' of https://github.com/JabRef/jabref into import…

931ad7e

…Bookends

Merge branch 'importBookends' of https://github.com/JabRef/jabref int…

ee98f2e

…o importBookends

Fix a few other NPE

571b464

Improve importer according to feedback from user

7bd56de

lenhard approved these changes Feb 16, 2018

View reviewed changes

Siedlerchr merged commit bb2b078 into master Feb 16, 2018

Siedlerchr deleted the importBookends branch February 16, 2018 15:05

Siedlerchr mentioned this pull request Feb 17, 2018

Refactor BibDatabase Migrations #3733

Merged

Uh oh!

Conversation

tobiasdiez commented Feb 11, 2018

Uh oh!

tobiasdiez commented Feb 12, 2018

Uh oh!

Siedlerchr commented Feb 12, 2018

Uh oh!

Siedlerchr commented Feb 12, 2018

Uh oh!

Siedlerchr commented Feb 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lenhard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lenhard left a comment

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented Feb 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants