Extend RIS import with multiple fields #3642

lenhard · 2018-01-17T13:33:39Z

Meeting time is programming time :)

This addresses the import related things described in #3634 #2607

I basically went through the lists linked by @dsifford in the first issue, tried to adapt our import if possible and added custom fields when not. Our importer tests are still running.

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

The changes look good. A few of the additional fields are actually standard biblatex fields (or have an equivalent one) and thus should be added to the FieldName enum. Apart from these minor points, this can be merged.

tobiasdiez · 2018-01-17T13:48:43Z

src/main/java/org/jabref/logic/importer/fileformat/RisImporter.java

+                    else if ("AV".equals(tag)) {
+                        fields.put("archive_location", value);
+                    } else if ("C3".equals(tag)) {
+                        fields.put("event", value);


Biblatex has eventtitle

tobiasdiez · 2018-01-17T13:49:28Z

src/main/java/org/jabref/logic/importer/fileformat/RisImporter.java

+                    } else if ("DB".equals(tag)) {
+                        fields.put("archive", value);
+                    } else if ("N1".equals(tag) || "RN".equals(tag)) {
+                        fields.put("note", value);


Note is a standard biblatex field and thus should be added to FieldName

tobiasdiez · 2018-01-17T13:50:04Z

src/main/java/org/jabref/logic/importer/fileformat/RisImporter.java

+                    } else if ("SE".equals(tag)) {
+                        fields.put("section", value);
+                    } else if ("ST".equals(tag)) {
+                        fields.put("shortTitle", value);


shorttitle is a standard biblatex field

shorttitle wasn't even part of FieldName so far. I have added it now.

tobiasdiez · 2018-01-17T13:52:38Z

src/main/java/org/jabref/logic/importer/fileformat/RisImporter.java

+                    } else if ("ST".equals(tag)) {
+                        fields.put("shortTitle", value);
+                    } else if ("C2".equals(tag)) {
+                        fields.put("pubmed-identifier", value);


I'm not sure if we are already have a PMID field. If not, the (biblatex) standard is to specify it in the format

eprint = {<pmid>}, eprinttype = {pubmed},

tobiasdiez · 2018-01-17T13:53:03Z

src/main/java/org/jabref/logic/importer/fileformat/RisImporter.java

+                    } else if ("C2".equals(tag)) {
+                        fields.put("pubmed-identifier", value);
+                    } else if ("TA".equals(tag)) {
+                        fields.put("translator", value);


translator is also a standard biblatex field

lenhard · 2018-01-17T14:07:08Z

@tobiasdiez Thanks a lot! I didn't have biblatex in mind when setting the fields. Your comments are all integrated.

Siedlerchr

LGTM, but I would really prefer if you could add a simple test

Siedlerchr · 2018-01-17T16:32:33Z

src/main/java/org/jabref/logic/importer/fileformat/RisImporter.java

                    } else if ("BT".equals(tag)) {
                        fields.put(FieldName.BOOKTITLE, value);
-                    } else if ("T2".equals(tag) && (fields.get(FieldName.JOURNAL) == null || "".equals(fields.get(FieldName.JOURNAL)))) {
+                    } else if (("T2".equals(tag) || "J2".equals(tag) || "JA".equals(tag)) && (fields.get(FieldName.JOURNAL) == null || "".equals(fields.get(FieldName.JOURNAL)))) {


This would have been optimal to use a switch case, which is very efficient for strings, but that's a bit of personal taste ;)

You are right in terms of efficiency, but I really don't like switches. It is just to easy to forget the break statement. That doesn't happen if you stick to if and always use braces.

lenhard · 2018-01-18T09:03:53Z

Is the RISImporterTestFiles class not executed during the travis build? Because those tests were actually partly broken. Anyway, I adapted them to the new import structure (which changes a few fields) and I added the RIS code from #3634 as a new test case.

Siedlerchr · 2018-01-18T09:41:19Z

Regarding the test, seems to be a general problem with the parameterized tests and the new junit ä.
I noticed the same with the office XML tests

lenhard · 2018-01-18T10:14:48Z

Ok, then this is something that really needs fixing. I'll open an issue for it. Otherwise this PR is ready to merge from my point of view.

lenhard · 2018-01-21T13:02:28Z

In the spirit of getting things done and because all reviewer comments have been addressed, I'll merge this now.

* upstream/master: Extend RIS import with multiple fields (#3642) Fix ICAR fetcher test which resulted in build failure (#3654)

* upstream/master: (46 commits) Replace openoffice jars with libreoffice jars (#3662) Export no empty lines in RIS format (#3661) Remove apache commons logging in favor of slf4j + log4j for JAVA 9 (#3653) fix isbn fetcher test as Joshua Bloch has published a new revivsion of Effetive Java convert to junit5 jupiter Extend RIS import with multiple fields (#3642) Fix ICAR fetcher test which resulted in build failure (#3654) New translations JabRef_en.properties (French) (#3650) Add link to MADR and fix typo [WIP] Add "Convert to BibTeX format" cleanup (#3541) Fix typo Fix typo Quickfix to get build running on all platforms (#3638) New translations JabRef_en.properties (French) New translations JabRef_en.properties (Vietnamese) New translations JabRef_en.properties (Chinese Simplified) New translations JabRef_en.properties (Dutch) New translations JabRef_en.properties (French) New translations JabRef_en.properties (German) New translations JabRef_en.properties (Greek) New translations JabRef_en.properties (Indonesian) ...

Extend RIS import with multiple fields

f9fa6d4

lenhard added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 17, 2018

tobiasdiez requested changes Jan 17, 2018

View reviewed changes

Refer to more biblatex types

70ce087

tobiasdiez approved these changes Jan 17, 2018

View reviewed changes

Siedlerchr approved these changes Jan 17, 2018

View reviewed changes

Update test files according to new importer

a67ef5a

lenhard mentioned this pull request Jan 18, 2018

Parameterized tests are not executed on travis #3645

Closed

lenhard merged commit 733da31 into master Jan 21, 2018

lenhard deleted the fix-ris-import branch January 21, 2018 13:02

This was referenced Jan 21, 2018

RIS import ignores L2 ("Full-text link") field #2607

Closed

Incorrect RIS fields #3634

Closed

Siedlerchr added a commit that referenced this pull request Jan 21, 2018

Merge remote-tracking branch 'upstream/master' into removecommonslogging

ea7744f

* upstream/master: Extend RIS import with multiple fields (#3642) Fix ICAR fetcher test which resulted in build failure (#3654)

lenhard mentioned this pull request Jan 25, 2018

Migrate importer tests to JUnit5 #3665

Merged

5 tasks

Uh oh!

Extend RIS import with multiple fields #3642

Extend RIS import with multiple fields #3642

Uh oh!

Conversation

lenhard commented Jan 17, 2018

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

tobiasdiez Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lenhard commented Jan 17, 2018

Uh oh!

Siedlerchr 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!

lenhard commented Jan 18, 2018

Uh oh!

Siedlerchr commented Jan 18, 2018

Uh oh!

lenhard commented Jan 18, 2018

Uh oh!

lenhard commented Jan 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tobiasdiez Jan 17, 2018 •

edited

Loading