Rewrite mods exporter and add test for it #1989

tschechlovdev · 2016-09-15T16:57:30Z

Regarding: #1920
Rewrite the MODS exporter with a JAXB parser.
Details on the format and the newest xml schema can be found here.
I've used the newest schema (version 3.6).
This PR uses the same gradle script and schema as in #1942 .

internal QS
Tests created for changes
Manually tested changed features in running JabRef

Siedlerchr · 2016-09-15T20:54:55Z

src/main/java/net/sf/jabref/logic/exporter/ModsExportFormat.java

+                for (Map.Entry<String, String> entry : fieldMap.entrySet()) {
+                    String key = entry.getKey();
+                    String value = entry.getValue();
+                    if (FieldName.AUTHOR.equals(key)) {


Please split the author handling code in a new method

Maybe, even net.sf.jabref.model.entry.AuthorList#parse can be directly used?

Siedlerchr · 2016-09-15T20:55:21Z

src/main/java/net/sf/jabref/logic/exporter/ModsExportFormat.java

+                            }
+                        }
+                        mods.getModsGroup().add(name);
+                    } else if ("affiliation".equals(key)) {


I think here a switch statement could be useful

Siedlerchr · 2016-09-15T20:56:40Z

src/main/java/net/sf/jabref/logic/exporter/ModsExportFormat.java

+
+                @Override
+                public String getPreferredPrefix(String namespaceUri, String suggestion, boolean requirePrefix) {
+                    if ("http://www.loc.gov/mods/v3".equals(namespaceUri)) {


Pleas consider extracting those as String constants, so in case of change they could be easily changed..

Siedlerchr · 2016-09-15T20:56:48Z

src/main/java/net/sf/jabref/logic/exporter/ModsExportFormat.java

+            //formate the output
+            marshaller.setProperty(Marshaller.JAXB_FORMATTED_OUTPUT, Boolean.TRUE);
+            marshaller.setProperty(Marshaller.JAXB_SCHEMA_LOCATION,
+                    "http://www.loc.gov/standards/mods/v3/mods-3-6.xsd");


same as above

tschechlovdev · 2016-09-20T14:20:39Z

For some reason codecov doesn't run the class ModsExportFormatTestFiles like in #1942 ...

Siedlerchr · 2016-09-21T07:14:25Z

@tschechlovdev Codecov seems to work again: codecov/project — 28.28% (+0.42%) compared to a8e1488

koppor · 2016-09-21T07:15:00Z

Please work on coverage - or is the coverage indication wrong?

tschechlovdev · 2016-09-21T09:27:01Z

No, I have two test classes. ModsExportFormatTest and ModsExportFormatTestFiles. The coverage comes from ModsExportFormatTest, but ModsExportFormatTestFiles doesn't seem to be executed.

I tried to rename ModsExportFormatTestFiles to ModsExportFormatFilesTest, because I had the intention that it is because the class didn't end with test. Though that also doesn't seem to work.

boceckts

Some work is still required. I would first fix the test files and once your tests are failing rework the appropriate methods.
In my opinionen it would also be a good idea to create a test to see if an mods exported xml file can be imported by the mods importer and if it produces the same bib entry.

boceckts · 2016-09-21T09:30:44Z

src/main/java/net/sf/jabref/logic/exporter/ModsExportFormat.java

+
+                    switch (key) {
+
+                    case FieldName.AUTHOR:


Indent the cases please.

boceckts · 2016-09-21T09:32:29Z

src/main/java/net/sf/jabref/logic/exporter/ModsExportFormat.java

+                    case FieldName.JOURNAL:
+                        addJournal(value, relatedItem);
+                        break;
+                    default:


default case is unnecessary here.

No! Codecov will complain about a case with no default. There always shoold be a default in a switch case

boceckts · 2016-09-21T09:34:59Z

src/main/java/net/sf/jabref/logic/exporter/ModsExportFormat.java

+            JAXBElement<ModsCollectionDefinition> jaxbElement = new JAXBElement<>(new QName("modsCollection"),
+                    ModsCollectionDefinition.class, modsCollection);
+
+


Why are there so many empty lines here, please use max one empty line to separate different code elements to groups.

boceckts · 2016-09-21T09:36:26Z

src/main/java/net/sf/jabref/logic/exporter/ModsExportFormat.java

+            }
+        };
+
+        //formate the output


Typo "format"

boceckts · 2016-09-21T09:47:11Z

src/main/java/net/sf/jabref/logic/exporter/ModsExportFormat.java

+                if (MODS_NAMESPACE_URI.equals(namespaceUri)) {
+                    return "mods";
+                }
+                return "";


Rplace with return MODS_NAMESPACE_URI.equals(namespaceUri) ? "mods" : "";

boceckts · 2016-09-21T10:07:32Z

src/test/resources/net/sf/jabref/logic/exporter/ModsExportFormatTestBook.bib

@@ -0,0 +1,6 @@
+@Book{
+isbn = {123456789},
+issn = {123456789},


Please use different numbers for isbn and issn.

boceckts · 2016-09-21T10:07:48Z

src/test/resources/net/sf/jabref/logic/exporter/ModsExportFormatTestAllFields.bib

+  pmid = {123456789},
+  created = {2000-10-10},
+  modified = {2000-10-10},
+  captured = {2000-10-10},


Please use different dates.

boceckts · 2016-09-21T10:11:27Z

src/test/resources/net/sf/jabref/logic/exporter/ModsExportFormatTestBook.xml

+        <mods:genre>book</mods:genre>
+        <mods:identifier type="issn">123456789</mods:identifier>
+        <mods:subject>
+            <topic>note; and another note;</topic>


I think the keywords should be separated and a new topic node should be created for each one instead of writing them in one node. Same as for the ModsExportFormatTestAllFields.xml.

boceckts · 2016-09-21T10:12:40Z

src/test/resources/net/sf/jabref/logic/exporter/ModsExportFormatTestIsbnIssn.bib

+  pages =    {1234},
+  author =   {Mustermann},
+  isbn =     {123456789},
+  issn =     {12345678},


Use different test numbers for isbn and issn here, too.

boceckts · 2016-09-21T10:19:49Z

src/test/resources/net/sf/jabref/logic/exporter/ModsExportFormatTestAllFields.xml

+            <namePart type="given"></namePart>
+            <namePart type="given">know</namePart>
+            <namePart type="family">Realy</namePart>
+            <namePart type="given"></namePart>


Why are there empty given name fields?

Siedlerchr · 2016-09-21T11:11:42Z

Regarding the test coverage. I have no idea, but maybe it is related to the fact that it's a parameterized test?

Siedlerchr · 2016-09-22T10:15:19Z

src/main/java/net/sf/jabref/logic/exporter/ModsExportFormat.java

+        int minusIndex = value.indexOf(minus);
+        String startPage = value.substring(0, minusIndex);
+        String endPage = "";
+        if ("-".equals(minus)) {


Could you please extrac thte Singe "-" and the double "--" to constants?

boceckts · 2016-09-22T14:04:18Z

Regarding the not executed tests #2017 (comment).

koppor · 2016-09-22T22:54:40Z

src/main/java/net/sf/jabref/logic/exporter/ModsExportFormat.java

+                }
+            } else {
+                //no "," indicates that there should only be a family name
+                namePart.setAtType("family");


Why no test coverage?

koppor · 2016-09-22T22:54:54Z

src/main/java/net/sf/jabref/logic/exporter/ModsExportFormat.java

+
+    private void addIdentifier(String key, String value, ModsDefinition mods) {
+        if ("citekey".equals(key)) {
+            mods.setID(value);


Why no test coverage?

koppor · 2016-09-22T22:55:07Z

src/main/java/net/sf/jabref/logic/exporter/ModsExportFormat.java

+        if ("-".equals(minus)) {
+            endPage = value.substring(minusIndex + 1);
+        } else if ("--".equals(minus)) {
+            endPage = value.substring(minusIndex + 2);


Why no test coverage?

koppor · 2016-09-22T23:03:22Z

Please increase test coverage.

I did not see a roundtrip test: bibtex -> mods -> bibtex and mods -> bibtex -> mods. Is that possible?

tschechlovdev · 2016-09-23T10:49:43Z

@koppor Regarding test coverage: It's still the same problem that the parameterized test are not run by codecov. I will take care of this in a separate PR.
Regarding the roundtrip test: I will add one.

tobiasdiez · 2016-09-23T13:36:23Z

I remove the "ready-for-review" for the moment until the comments are addressed. Please add it again afterwards. Thanks.

# Conflicts: # src/main/java/net/sf/jabref/logic/mods/MODSEntry.java # src/main/java/net/sf/jabref/logic/msbib/MSBibEntry.java

Siedlerchr · 2016-09-27T16:30:42Z

Good! I merge in!

obraliar · 2016-09-27T17:24:35Z

I'm getting the following error:

:compileJava
/home/admir/tmp/jabref/src/main/java/net/sf/jabref/logic/exporter/ModsExportFormat.java:60:
error: incompatible types: Character cannot be converted to String
            .getKeywordSeparator();

OS data:

Distributor ID: Debian
Description:    Debian GNU/Linux 8.5 (jessie)
Release:        8.5
Codename:       jessie

Siedlerchr · 2016-09-27T18:06:20Z

I'll take a quick look

Siedlerchr · 2016-09-27T18:17:05Z

@obraliar Works fine. Did you run ./gradlew eclipse (or build whatever youride is) ?
And also ./gradlew check executes fine. No related error.

obraliar · 2016-09-27T18:42:33Z

@Siedlerchr What OS are you using?
./gradlew assemble and also ./gradlew check fails with the same error. I cloned jabref into another directory to annul all factors. Unfortunately, it doesen't work. It works when resetting to the commit before.

Siedlerchr · 2016-09-27T18:44:27Z

@obraliar Now it get the same. Seems like my local repo was not update to date. I will revert that commit here. Problem is the internal class stuff. @tschechlovdev Sorry, but you have to fix this

Siedlerchr · 2016-09-27T18:53:06Z

@obraliar I just reverted the commit. Follow up discussion in #2078

* rewrite mods exporter and add test for it * some code refactorings and editing the test * add test case * delete old mods helper classes and move other used classes * rename testfiles test class * address comments * address comments

rewrite mods exporter and add test for it

be4ecdb

tschechlovdev added the stupro label Sep 15, 2016

Siedlerchr reviewed Sep 15, 2016

View reviewed changes

tschechlovdev force-pushed the mods-export-test branch 3 times, most recently from 6e0956f to 2c31d3d Compare September 19, 2016 17:17

tschechlovdev added the stupro-ready-for-internal-review label Sep 19, 2016

tschechlovdev force-pushed the mods-export-test branch 2 times, most recently from a756032 to b5e6183 Compare September 20, 2016 12:23

some code refactorings and editing the test

5878b95

tschechlovdev force-pushed the mods-export-test branch from b5e6183 to 5878b95 Compare September 20, 2016 12:58

tschechlovdev added 2 commits September 20, 2016 15:00

Merge remote-tracking branch 'upstream/master' into mods-export-test

f0613ad

add test case

19b3e8e

delete old mods helper classes and move other used classes

63c471e

tschechlovdev force-pushed the mods-export-test branch from 9b8b1d1 to 63c471e Compare September 20, 2016 14:44

rename testfiles test class

73d39f2

boceckts suggested changes Sep 21, 2016

View reviewed changes

boceckts removed the stupro-ready-for-internal-review label Sep 21, 2016

address comments

ff7215e

tschechlovdev added the stupro-ready-for-internal-review label Sep 21, 2016

Siedlerchr reviewed Sep 22, 2016

View reviewed changes

boceckts approved these changes Sep 22, 2016

View reviewed changes

boceckts added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 22, 2016

boceckts removed the stupro-ready-for-internal-review label Sep 22, 2016

boceckts changed the title ~~[WIP] Rewrite mods exporter and add test for it~~ Rewrite mods exporter and add test for it Sep 22, 2016

koppor reviewed Sep 22, 2016

View reviewed changes

tobiasdiez removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 23, 2016

tschechlovdev added 2 commits September 27, 2016 16:16

address comments

a554e07

Merge remote-tracking branch 'upstream/master' into mods-export-test

8cab54c

# Conflicts: # src/main/java/net/sf/jabref/logic/mods/MODSEntry.java # src/main/java/net/sf/jabref/logic/msbib/MSBibEntry.java

tschechlovdev added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 27, 2016

Siedlerchr merged commit 4236e3f into JabRef:master Sep 27, 2016

This was referenced Sep 27, 2016

Revert "Rewrite mods exporter and add test for it" #2077

Merged

Namespace in jaxb should not rely on internal java com.sun classes #2078

Closed

tschechlovdev mentioned this pull request Sep 28, 2016

[WIP] Rewrite mods exporter and add test #2088

Merged

2 tasks

		JAXBElement<ModsCollectionDefinition> jaxbElement = new JAXBElement<>(new QName("modsCollection"),
		ModsCollectionDefinition.class, modsCollection);

Rewrite mods exporter and add test for it #1989

Rewrite mods exporter and add test for it #1989

Conversation

tschechlovdev commented Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tschechlovdev commented Sep 20, 2016

Siedlerchr commented Sep 21, 2016

koppor commented Sep 21, 2016

tschechlovdev commented Sep 21, 2016 • edited Loading

boceckts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boceckts Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boceckts Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr commented Sep 21, 2016

Choose a reason for hiding this comment

boceckts commented Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor commented Sep 22, 2016

tschechlovdev commented Sep 23, 2016

tobiasdiez commented Sep 23, 2016

Siedlerchr commented Sep 27, 2016

obraliar commented Sep 27, 2016 • edited Loading

Siedlerchr commented Sep 27, 2016

Siedlerchr commented Sep 27, 2016

obraliar commented Sep 27, 2016

Siedlerchr commented Sep 27, 2016

Siedlerchr commented Sep 27, 2016

tschechlovdev commented Sep 15, 2016 •

edited

Loading

tschechlovdev commented Sep 21, 2016 •

edited

Loading

boceckts Sep 21, 2016 •

edited

Loading

boceckts Sep 21, 2016 •

edited

Loading

boceckts commented Sep 22, 2016 •

edited

Loading

obraliar commented Sep 27, 2016 •

edited

Loading