Work on CitationSTyleGeneratorTest (and fix PagesChecker)

Co-authored-by: Christoph <siedlerkiller@gmail.com> Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com> Co-authored-by: ThiloteE <73715071+ThiloteE@users.noreply.github.com>
JabRef · Mar 28, 2022 · d148a54 · d148a54
1 parent 0354542
commit d148a54
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 29 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -40,6 +40,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 - We fixed an issue where opening the changelog from withing JabRef led to a 404 error [#8563](https://github.com/JabRef/jabref/issues/8563)
 - We fixed an issue where not all found unlinked local files were imported correctly due to some race condition. [#8444](https://github.com/JabRef/jabref/issues/8444)
 - We fixed an issue where Merge entries dialog exceeds screen boundaries.
+- We fixed the page ranges checker to detect article numbers (used at [Check Integrity](https://docs.jabref.org/finding-sorting-and-cleaning-entries/checkintegrity)).
 
 ### Removed
 

diff --git a/src/main/java/org/jabref/logic/citationstyle/CSLAdapter.java b/src/main/java/org/jabref/logic/citationstyle/CSLAdapter.java
@@ -10,13 +10,15 @@
 import java.util.Set;
 
 import org.jabref.logic.formatter.bibtexfields.RemoveNewlinesFormatter;
+import org.jabref.logic.integrity.PagesChecker;
 import org.jabref.model.database.BibDatabaseContext;
 import org.jabref.model.entry.BibEntry;
 import org.jabref.model.entry.BibEntryType;
 import org.jabref.model.entry.BibEntryTypesManager;
 import org.jabref.model.entry.Month;
 import org.jabref.model.entry.field.Field;
 import org.jabref.model.entry.field.StandardField;
+import org.jabref.model.entry.types.StandardEntryType;
 import org.jabref.model.strings.LatexToUnicodeAdapter;
 
 import de.undercouch.citeproc.CSL;
@@ -99,7 +101,10 @@ private static class JabRefItemDataProvider implements ItemDataProvider {
         /**
          * Converts the {@link BibEntry} into {@link CSLItemData}.
          */
-        private static CSLItemData bibEntryToCSLItemData(BibEntry bibEntry, BibDatabaseContext bibDatabaseContext, BibEntryTypesManager entryTypesManager) {
+        private static CSLItemData bibEntryToCSLItemData(BibEntry originalBibEntry, BibDatabaseContext bibDatabaseContext, BibEntryTypesManager entryTypesManager) {
+            // We need to make a deep copy, because we modify the entry according to the logic presented at
+            // https://github.com/JabRef/jabref/issues/8372#issuecomment-1014941935
+            BibEntry bibEntry = (BibEntry) originalBibEntry.clone();
 
             String citeKey = bibEntry.getCitationKey().orElse("");
             BibTeXEntry bibTeXEntry = new BibTeXEntry(new Key(bibEntry.getType().getName()), new Key(citeKey));
@@ -109,8 +114,41 @@ private static CSLItemData bibEntryToCSLItemData(BibEntry bibEntry, BibDatabaseC
 
             Optional<BibEntryType> entryType = entryTypesManager.enrich(bibEntry.getType(), bibDatabaseContext.getMode());
 
-            Set<Field> fields = entryType.map(BibEntryType::getAllFields).orElse(bibEntry.getFields());
+            if (bibEntry.getType().equals(StandardEntryType.Article)) {
+                // Patch bibEntry to contain the right BibTeX (not BibLaTeX) fields
+                // Note that we do not need to convert from "pages" to "page", because CiteProc already handles it
+                // See BibTeXConverter
+                if (bibDatabaseContext.isBiblatexMode()) {
+                    // Move "number" to "issue" if "issue" does not exist
+                    Optional<String> numberField = bibEntry.getField(StandardField.NUMBER);
+                    numberField.ifPresent(number -> {
+                        if (!bibEntry.hasField(StandardField.ISSUE)) {
+                            bibEntry.setField(StandardField.ISSUE, number);
+                            bibEntry.clearField(StandardField.NUMBER);
+                        }
+                    });
+                    bibEntry.getField(StandardField.EID).ifPresent(eid -> {
+                        bibEntry.setField(StandardField.NUMBER, eid);
+                        bibEntry.clearField(StandardField.EID);
+                    });
+                } else {
+                    bibEntry.getField(StandardField.NUMBER).ifPresent(number -> {
+                        bibEntry.setField(StandardField.ISSUE, number);
+                        bibEntry.clearField(StandardField.NUMBER);
+                    });
+                    bibEntry.getField(StandardField.PAGES).ifPresent(pages -> {
+                        if (new PagesChecker(bibDatabaseContext).checkValue(pages).isPresent()) {
+                            // pages field contains no valid pages range
+                            if (!pages.startsWith("Article")) {
+                                bibEntry.setField(StandardField.PAGES, "Article " + pages);
+                            }
+                        }
+                    });
+                }
+            }
 
+            Set<Field> fields = entryType.map(BibEntryType::getAllFields).orElse(bibEntry.getFields());
+            fields.addAll(bibEntry.getFields());
             for (Field key : fields) {
                 bibEntry.getResolvedFieldOrAlias(key, bibDatabaseContext.getDatabase())
                         .map(removeNewlinesFormatter::format)

diff --git a/src/main/java/org/jabref/logic/integrity/PagesChecker.java b/src/main/java/org/jabref/logic/integrity/PagesChecker.java
@@ -1,5 +1,6 @@
 package org.jabref.logic.integrity;
 
+import java.util.Arrays;
 import java.util.Optional;
 import java.util.function.Predicate;
 import java.util.regex.Pattern;
@@ -11,28 +12,26 @@
 public class PagesChecker implements ValueChecker {
 
     private static final String PAGES_EXP_BIBTEX = ""
-            + "\\A"                 // begin String
+            + "\\A"               // begin String
             + "("
             + "[A-Za-z]?\\d*"     // optional prefix and number
             + "("
-            + "\\+|-{2}"    // separator
-            + "[A-Za-z]?\\d*" // optional prefix and number
+            + "\\+|-{2}"          // separator
+            + "[A-Za-z]?\\d*"     // optional prefix and number
             + ")?"
             + ",?"                // page range separation
             + ")*"
-            + "\\z";                // end String
+            + "\\z";              // end String
 
+    // See https://packages.oth-regensburg.de/ctan/macros/latex/contrib/biblatex/doc/biblatex.pdf#subsubsection.3.15.3 for valid content
     private static final String PAGES_EXP_BIBLATEX = ""
-            + "\\A"                 // begin String
-            + "("
+            + "\\A"               // begin String
             + "[A-Za-z]?\\d*"     // optional prefix and number
             + "("
-            + "\\+|-{1,2}|\u2013" // separator
-            + "[A-Za-z]?\\d*" // optional prefix and number
+            + "(\\+|-{1,2}|\u2013)" // separator
+            + "[A-Za-z]?\\d*"     // optional prefix and number
             + ")?"
-            + ",?"                // page range separation
-            + ")*"
-            + "\\z";                // end String
+            + "\\z";              // end String
 
     private final Predicate<String> isValidPageNumber;
 
@@ -59,10 +58,13 @@ public Optional<String> checkValue(String value) {
             return Optional.empty();
         }
 
-        if (!isValidPageNumber.test(value.trim())) {
+        String[] split = value.split(",");
+
+        if (Arrays.stream(split)
+                .map(String::trim)
+                .anyMatch(pageRange -> !isValidPageNumber.test(pageRange))) {
             return Optional.of(Localization.lang("should contain a valid page number range"));
         }
-
         return Optional.empty();
     }
 }
diff --git a/src/test/java/org/jabref/logic/citationstyle/CitationStyleGeneratorTest.java b/src/test/java/org/jabref/logic/citationstyle/CitationStyleGeneratorTest.java
@@ -140,11 +140,11 @@ static Stream<Arguments> testCslMapping() {
         // if the default citation style changes this has to be modified
         return Stream.of(
                 Arguments.of(
-                        "[1]F. Last and J. Doe, Art. no. 7.\n",
+                        "[1]F. Last and J. Doe, no. 28.\n",
                         BibDatabaseMode.BIBTEX,
                         new BibEntry(StandardEntryType.Article)
                                 .withField(StandardField.AUTHOR, "Last, First and\nDoe, Jane")
-                                .withField(StandardField.NUMBER, "7"),
+                                .withField(StandardField.NUMBER, "28"),
                         "ieee.csl"),
                 Arguments.of(
                         "[1]F. Last and J. Doe, no. 7.\n",
@@ -154,11 +154,11 @@ static Stream<Arguments> testCslMapping() {
                                 .withField(StandardField.ISSUE, "7"),
                         "ieee.csl"),
                 Arguments.of(
-                        "[1]F. Last and J. Doe, Art. no. 7.\n",
+                        "[1]F. Last and J. Doe, no. 28.\n",
                         BibDatabaseMode.BIBLATEX,
                         new BibEntry(StandardEntryType.Article)
                                 .withField(StandardField.AUTHOR, "Last, First and Doe, Jane")
-                                .withField(StandardField.NUMBER, "7"),
+                                .withField(StandardField.NUMBER, "28"),
                         "ieee.csl"),
                 Arguments.of(
                         "[1]F. Last and J. Doe, no. 7, Art. no. 28.\n",
@@ -169,12 +169,12 @@ static Stream<Arguments> testCslMapping() {
                                 .withField(StandardField.NUMBER, "28"),
                         "ieee.csl"),
                 Arguments.of(
-                        "[1]F. Last and J. Doe, no. 28, pp. 7–8.\n",
+                        "[1]F. Last and J. Doe, no. 33, pp. 7–8.\n",
                         BibDatabaseMode.BIBLATEX,
                         new BibEntry(StandardEntryType.Article)
                                 .withField(StandardField.AUTHOR, "Last, First and\nDoe, Jane")
                                 .withField(StandardField.PAGES, "7--8")
-                                .withField(StandardField.ISSUE, "28"),
+                                .withField(StandardField.ISSUE, "33"),
                         "ieee.csl"),
 
                 Arguments.of(
@@ -192,7 +192,7 @@ static Stream<Arguments> testCslMapping() {
                         "apa-6th-edition.csl"),
 
                 Arguments.of(
-                        "Foo, B. (n.d.). volume + issue + pages. Bib(La)TeX Journal, 1, 45–67.\n",
+                        "Foo, B. (n.d.). volume + issue + pages. Bib(La)TeX Journal, 1(9), 45–67.\n",
                         BibDatabaseMode.BIBTEX,
                         new BibEntry(StandardEntryType.Article)
                                 .withField(StandardField.AUTHOR, "Foo, Bar")
@@ -201,7 +201,7 @@ static Stream<Arguments> testCslMapping() {
                                 .withField(StandardField.TITLE, "volume + issue + pages")
                                 .withField(StandardField.VOLUME, "1")
                                 .withField(StandardField.COMMENT, "The issue field does not exist in Bibtex standard, therefore there is no need to render it (The issue field exists in biblatex standard though.). Since, for this entry, there is no number field present and therefore no data will be overwriten, enabling the user to be able to move the data within the issue field to the number field via cleanup action is something worth pursuing.")
-                                .withField(StandardField.ISSUE, "9issue"),
+                                .withField(StandardField.ISSUE, "9"),
                         "apa-6th-edition.csl"),
 
                 Arguments.of(
@@ -227,19 +227,19 @@ static Stream<Arguments> testCslMapping() {
 
                 // The issue field does not exist in bibtex standard, therefore there is no need to render it (it exists in biblatex standard though). Since, for this entry, there is no number field present and therefore no data will be overwriten, enabling the user to be able to move the data within the issue field to the number field via cleanup action is something worth pursuing.
                 Arguments.of(
-                        "Foo, B. (n.d.). issue + pages. Bib(La)TeX Journal, 45–67.\n",
+                        "Foo, B. (n.d.). issue + pages. Bib(La)TeX Journal, (9), 45–67.\n",
                         BibDatabaseMode.BIBTEX,
                         new BibEntry(StandardEntryType.Article)
                                 .withField(StandardField.AUTHOR, "Foo, Bar")
                                 .withField(StandardField.JOURNAL, "Bib(La)TeX Journal")
                                 .withField(StandardField.PAGES, "45--67")
                                 .withField(StandardField.TITLE, "issue + pages")
-                                .withField(StandardField.ISSUE, "9issue"),
+                                .withField(StandardField.ISSUE, "9"),
                         "apa-6th-edition.csl"),
 
                 // The issue field does not exist in bibtex standard, therefore there is no need to render it (it exists in biblatex standard though)
                 Arguments.of(
-                        "Foo, B. (n.d.). issue + number. Bib(La)TeX Journal, (3number), 45–67.\n",
+                        "Foo, B. (n.d.). issue + number. Bib(La)TeX Journal, (3number).\n",
                         BibDatabaseMode.BIBTEX,
                         new BibEntry(StandardEntryType.Article)
                                 .withField(StandardField.AUTHOR, "Foo, Bar")
@@ -271,12 +271,13 @@ static Stream<Arguments> testCslMapping() {
                                 .withField(StandardField.JOURNAL, "Bib(La)TeX Journal")
                                 .withField(StandardField.NUMBER, "3number")
                                 .withField(StandardField.PAGES, "Article 777")
-                                .withField(StandardField.TITLE, "number + article-number WITH the word article instead of pagerange"),
+                                .withField(StandardField.TITLE, "number + pages")
+                                .withField(StandardField.COMMENT, "number + article-number WITH the word article instead of pagerange"),
                         "apa-6th-edition.csl"),
 
                 // "The issue field does not exist in bibtex standard, therefore there is no need to render it (it exists in biblatex standard though). Since, for this entry, there is no number field present and therefore no data will be overwriten, enabling the user to be able to move the data within the issue field to the number field via cleanup action is something worth pursuing."
                 Arguments.of(
-                        "Foo, B. (n.d.). issue. Bib(La)TeX Journal.\n",
+                        "Foo, B. (n.d.). issue. Bib(La)TeX Journal, (9issue).\n",
                         BibDatabaseMode.BIBTEX,
                         new BibEntry(StandardEntryType.Article)
                                 .withField(StandardField.AUTHOR, "Foo, Bar")
@@ -305,7 +306,8 @@ static Stream<Arguments> testCslMapping() {
                                 .withField(StandardField.JOURNAL, "Bib(La)TeX Journal")
                                 .withField(StandardField.NUMBER, "3number")
                                 .withField(StandardField.PAGES, "777e23")
-                                .withField(StandardField.TITLE, "number + article-number WITHOUT the word article instead of pagerange"),
+                                .withField(StandardField.TITLE, "number + pages")
+                                .withField(StandardField.COMMENT, "number + article-number WITHOUT the word article instead of pagerange"),
                         "apa-6th-edition.csl"),
 
                 // Not rendering the "eid" field here, is correct. The "eid" field(short for: electronic identifier) does not exist in Bibtex standard. It exists in Biblatex standard though and is the field, in which the "article number" should be entered into. "Article number" is a field that does not exist in Bibtex and also not in Biblatex standard. As a workaround, some Journals have opted to put the article number into the pages field. APA 7th Style recommends following procedure: "If the journal article has an article number instead of a page range, include the word "Article" and then the article number instead of the page range." - Source: https://apastyle.apa.org/style-grammar-guidelines/references/examples/journal-article-references#2. Additionally the APA style (7th edition) created by the CSL community "prints the issue (= the "number" field" in Biblatex)  whenever it is in the data and the number (= the "eid" field in Biblatex) when no page range is present, entirely independent of the issue number" - Source: https://github.com/citation-style-language/styles/issues/5827#issuecomment-1006011280). I personally think the "eid" field SHOULD be rendered here SOMEWHERE, maybe even IN ADDITION to the page range, because we have the data, right? Why not show it? - But this is just my humble opinion and may not be coherent with the current APA Style 7th edition. Not rendering the "issue" field here is sufficient for APA 7th edition. Under current circumstances the "number" field takes priority over the "issue" field (see https://github.com/JabRef/jabref/issues/8372#issuecomment-1023768144). [Keyword: IS RENDERING BOTH VIABLE?]. Ideally, they would coexist: "Roughly speaking number subdivides volume and issue is much closer to subdividing year. I don't think I would want to say that issue is subordinate to number or vice versa. They sort of operate on a similar level." (Source: https://github.com/plk/biblatex/issues/726#issuecomment-1010264258)

diff --git a/src/test/java/org/jabref/logic/integrity/PagesCheckerBibLatexTest.java b/src/test/java/org/jabref/logic/integrity/PagesCheckerBibLatexTest.java
@@ -27,6 +27,11 @@ void acceptsSinglePage() {
         assertEquals(Optional.empty(), checker.checkValue("12"));
     }
 
+    @Test
+    void rejectsHexNumber() {
+        assertNotEquals(Optional.empty(), checker.checkValue("777e23"));
+    }
+
     @Test
     void acceptsSinglePageRange() {
         assertEquals(Optional.empty(), checker.checkValue("12-15"));