Fix biblatex cleanup for date field #2339

Siedlerchr · 2016-12-04T19:50:34Z

This fixes #2335
Moved biblatex Cleanup to new test

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

Moved biblatex Cleanup to new test

Siedlerchr · 2016-12-04T19:58:34Z

Failing test is something from Medline, not related

tobiasdiez

In general LGTM but I have a few remarks.

tobiasdiez · 2016-12-04T21:02:12Z

src/main/java/net/sf/jabref/logic/cleanup/BiblatexCleanup.java

+        if (entry.getField(FieldName.DATE).isPresent()) {
+            date = entry.getField(FieldName.DATE).get();
+        }
+        if (date.isEmpty()) {


I think if (StringUtils.isBlank (entry.getField(FieldName.DATE))) is cleaner (not sure about the naming)

I used this approach, because it captures 2 purposes and checks the optonal
a) If the field is not present the date is empty
b) if the field is present, we assign the value, however, it still might be empty

tobiasdiez · 2016-12-04T21:03:54Z

src/main/java/net/sf/jabref/logic/cleanup/BiblatexCleanup.java

+                Optional<String> oldYear = entry.getField(FieldName.YEAR);
+                Optional<String> oldMonth = entry.getField(FieldName.MONTH);
+
+                entry.setField(FieldName.DATE, newDate);


If I remember correctly, then set/clearField already return the right FieldChange

That was part of the code I didn't touch

tobiasdiez · 2016-12-04T21:04:31Z

src/test/java/net/sf/jabref/logic/cleanup/BiblatexCleanupTest.java

+
+    @Before
+    public void setUp() throws IOException {
+


Remove empty line

tobiasdiez · 2016-12-04T21:06:51Z

src/test/java/net/sf/jabref/logic/cleanup/BiblatexCleanupTest.java

+    public void setUp() throws IOException {
+
+        MetaData metaData = new MetaData();
+        BibDatabaseContext context = new BibDatabaseContext(new BibDatabase(), metaData, bibFolder.newFile("test.bib"));


Do we really need a create a file?

tobiasdiez · 2016-12-04T21:29:17Z

src/test/java/net/sf/jabref/logic/cleanup/BiblatexCleanupTest.java

+        BibEntry entry = new BibEntry();
+        entry.setField(FieldName.JOURNAL, "test");
+
+        worker.cleanup(preset, entry);


directly use the BiblatexCleanup.cleanup method instead of using the worker (makes tests and initialization more transparent)

tobiasdiez · 2016-12-04T21:32:29Z

src/test/java/net/sf/jabref/logic/cleanup/CleanupWorkerTest.java

@@ -308,17 +308,6 @@ public void cleanupLatexMergesTwoLatexMathEnvironments() {
    }

    @Test
-    public void convertToBiblatexMovesJournalToJournalTitle() {


I would leave this test here to make sure that the CleanupWorker works correctly wrt to the "Convert to BibLatex" option; the other test class for the BibLatexCleanupJob then only tests this class directly.

Minimize test

tobiasdiez · 2016-12-05T15:19:37Z

src/main/java/net/sf/jabref/logic/cleanup/BiblatexCleanup.java

-
-                    entry.clearField(oldFieldName);
-                    changes.add(new FieldChange(entry, oldFieldName, oldValue, null));
+                    changes.add(entry.setField(newFieldName, oldValue).orElse(null));


entry.setField(...).ifPresent(changes::add), otherwise you add null values to the list which is not so nice.

tobiasdiez · 2016-12-05T15:21:20Z

src/main/java/net/sf/jabref/logic/cleanup/BiblatexCleanup.java

        // Dates: create date out of year and month, save it and delete old fields
        // If there already exists a value for the field date, it is not overwritten
        String date = "";
        if (entry.getField(FieldName.DATE).isPresent()) {
            date = entry.getField(FieldName.DATE).get();
        }
-        if (date.isEmpty()) {
+        if (StringUtils.isBlank(date)) {


isBlank also accepts an optional I think (if not, please add a corresponding overload), hence Optional<String> date = entry.getField(FieldName.DATE) should work.

Siedlerchr · 2016-12-05T16:11:04Z

However in this case that would result in the original bug again, when checked for ifPresent ! If the field is not yet in the biblatex entry (which is obviously the case when you only have year), it returns an EmtyOptional and thus you would have the same situation as in the original code. This is why I splitted it.

* upstream/master: builds.jabref.org: Keep html files and just remove *.exe, *.dmg, *.jar during upload of a new build Fix comment typo Fix typos in comments in .travis.yml Switch to LGoodDatePicker (#2340) Gradle build scans should only run on TravisCI, not on the other CI services Try scans.gradle.com Remove reference to GPL - we are MIT now fix date changes in medline fetcher results Fix #2336: shutdown is working again (#2338)

Siedlerchr · 2016-12-05T18:11:58Z

Failing Snap CI is due to HTTP error 502 in AstrophysicsDataSystemTest

koppor · 2016-12-05T18:22:32Z

Could you please merge master again - I merged #2333 and thus SnapCI only does the basic tests as described at https://github.com/JabRef/jabref/wiki/CI#snapci 😇

* upstream/master: Update com.github.johnrengelman.shadow from 1.2.3 to 1.2.4 Introduce categories: DatabseTests, FetcherTests, GUITests Place oracle stub into src/main/java Move all tests into src/test/java

tobiasdiez · 2016-12-05T20:24:29Z

@Siedlerchr: sorry I don't understand you comment.
Lets go through this case by case with the following code:

Optional<String> date = entry.getField(FieldName.DATE);
Boolean isBlank = StringUtils.isBlank(date);

case	date	isBlank
no date at all	Optional.empty	true
empty string	Optional.of("")	true
something else	Optional.of("something")	false

You want to move year and month to the date field exactly in the first two cases, i.e., if and only if isBlank == true. What do I miss?

Siedlerchr · 2016-12-06T08:17:50Z

Ah now I understand your comment, the first case was not obvious for me, that an empty should return true.

* upstream/master: Remove obsolete variable Translate new messages (#2341) Update gradle from 3.2 to 3.2.1

* upstream/master: Move files to subfolders in file directory (#1899) # Conflicts: # src/test/java/net/sf/jabref/logic/cleanup/CleanupWorkerTest.java

Siedlerchr · 2016-12-07T20:23:07Z

@tobiasdiez The confusion came from the naming stuff. I previously had the apache.commons StringUtil class and not the jabref specific one. Changed that.

koppor · 2016-12-07T21:11:19Z

src/test/java/net/sf/jabref/logic/cleanup/BiblatexCleanupTest.java

+public class BiblatexCleanupTest {
+
+    @Rule
+    public TemporaryFolder bibFolder = new TemporaryFolder();


Is this variable ever used?

koppor · 2016-12-07T21:11:56Z

Besides the micro comment: LGTM 👍

tobiasdiez

One small comment, otherwise can be merged.

tobiasdiez · 2016-12-07T22:33:11Z

src/test/java/net/sf/jabref/logic/cleanup/CleanupWorkerTest.java

@@ -307,11 +308,11 @@ public void cleanupLatexMergesTwoLatexMathEnvironments() {
    public void convertToBiblatexMovesJournalToJournalTitle() {
        CleanupPreset preset = new CleanupPreset(CleanupPreset.CleanupStep.CONVERT_TO_BIBLATEX);
        BibEntry entry = new BibEntry();
-        entry.setField("journal", "test");
+        entry.setField(FieldName.JOURNAL, "test");


Please revert these changes, as hard-coded strings in tests are actually desirable. The logic for the conversation moves FieldName.JOURNAL to FieldName,JOURNALTITLE. Hence if somebody changes FieldName.JOURNAL = "stupid" the test still passes but the functionality is broken.

* upstream/master: Some more formatting Keeping shared database changes together and remove duplicate Add missing newline Update mockito-core from 2.2.26 to 2.2.28 Fix comments Add comment where to find documentation why swingx is broken # Conflicts: # CHANGELOG.md

* Fix biblatex cleanup for date field Moved biblatex Cleanup to new test * Add changelog entry * Directly use returend fieldChanges from set/Clear entry Minimize test * Use ifPresent with changes add * Use jabref StringUtil with Optional checker * Change field constants to strings in tests * Replace remaining field names with string * Remove unnecessary import

Siedlerchr added 2 commits December 4, 2016 20:47

Fix biblatex cleanup for date field

ff5f1b5

Moved biblatex Cleanup to new test

Add changelog entry

ee6e40f

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 4, 2016

tobiasdiez requested changes Dec 4, 2016

View reviewed changes

Directly use returend fieldChanges from set/Clear entry

5cb09cd

Minimize test

tobiasdiez reviewed Dec 5, 2016

View reviewed changes

Siedlerchr added 2 commits December 5, 2016 17:21

Use ifPresent with changes add

65454d9

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

5610e22

* upstream/master: Update com.github.johnrengelman.shadow from 1.2.3 to 1.2.4 Introduce categories: DatabseTests, FetcherTests, GUITests Place oracle stub into src/main/java Move all tests into src/test/java

Siedlerchr added 3 commits December 7, 2016 21:02

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

efdf747

* upstream/master: Remove obsolete variable Translate new messages (#2341) Update gradle from 3.2 to 3.2.1

Use jabref StringUtil with Optional checker

e09a43b

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

49d0e72

* upstream/master: Move files to subfolders in file directory (#1899) # Conflicts: # src/test/java/net/sf/jabref/logic/cleanup/CleanupWorkerTest.java

koppor reviewed Dec 7, 2016

View reviewed changes

tobiasdiez requested changes Dec 7, 2016

View reviewed changes

Siedlerchr and others added 4 commits December 8, 2016 09:17

Change field constants to strings in tests

38c9488

Replace remaining field names with string

07554e5

Remove unnecessary import

188838f

tobiasdiez merged commit 123f2e6 into master Dec 8, 2016

tobiasdiez deleted the biblatexDateCleanup branch December 8, 2016 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix biblatex cleanup for date field #2339

Fix biblatex cleanup for date field #2339

Siedlerchr commented Dec 4, 2016 •

edited

Loading

Siedlerchr commented Dec 4, 2016

tobiasdiez left a comment

tobiasdiez Dec 4, 2016

Siedlerchr Dec 5, 2016

tobiasdiez Dec 4, 2016

Siedlerchr Dec 5, 2016

tobiasdiez Dec 4, 2016

tobiasdiez Dec 4, 2016

tobiasdiez Dec 4, 2016

tobiasdiez Dec 4, 2016

tobiasdiez Dec 5, 2016

tobiasdiez Dec 5, 2016

Siedlerchr commented Dec 5, 2016 via email •

edited

Loading

Siedlerchr commented Dec 5, 2016

koppor commented Dec 5, 2016

tobiasdiez commented Dec 5, 2016 •

edited

Loading

Siedlerchr commented Dec 6, 2016

Siedlerchr commented Dec 7, 2016

koppor Dec 7, 2016

koppor commented Dec 7, 2016

tobiasdiez left a comment

tobiasdiez Dec 7, 2016

Fix biblatex cleanup for date field #2339

Fix biblatex cleanup for date field #2339

Conversation

Siedlerchr commented Dec 4, 2016 • edited Loading

Siedlerchr commented Dec 4, 2016

tobiasdiez 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

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

Siedlerchr commented Dec 5, 2016 via email • edited Loading

Siedlerchr commented Dec 5, 2016

koppor commented Dec 5, 2016

tobiasdiez commented Dec 5, 2016 • edited Loading

Siedlerchr commented Dec 6, 2016

Siedlerchr commented Dec 7, 2016

Choose a reason for hiding this comment

koppor commented Dec 7, 2016

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr commented Dec 4, 2016 •

edited

Loading

Siedlerchr commented Dec 5, 2016 via email •

edited

Loading

tobiasdiez commented Dec 5, 2016 •

edited

Loading