Add parser for German months (#3536) #3734

Eiswindyeti · 2018-02-17T13:49:07Z

Changed the month parse that it can also parse German months now.
This should solve issue #3536.
Have I missed something?

(For PE1718: Name: Johannes Preßmar)

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?)

Siedlerchr · 2018-02-17T14:29:14Z

CHANGELOG.md

@@ -34,6 +34,7 @@ The new default removes the linked file from the entry instead of deleting the f
 - The group editing window can now also be called by double-clicking the group to be edited. [koppor#277](https://github.com/koppor/jabref/issues/277)
 - The magnifier icon at the search shows the [search mode](https://help.jabref.org/en/Search#search-modes) again. [#3535](https://github.com/JabRef/jabref/issues/3535)
 - We added a new cleanup operation that replaces ligatures with their expanded form. [#3613](https://github.com/JabRef/jabref/issues/3613)
+- We added the function to parse German month names.


Coressponding issue?

Sorry I forgot. Fixed it.

Siedlerchr · 2018-02-17T14:42:58Z

src/main/java/org/jabref/model/entry/Month.java

+         * @author Johannes Preßmar
+	 */
+	private static Optional<Month> parseGermanShortMonth(String value) {
+		String[] shortMonths = new DateFormatSymbols(Locale.GERMAN).getShortMonths();


Better use the new java 8 Date & Time Api

Construt a DateTimeFormatter.ofPattern("MMM", Locale.DE)
Use YearMonth.parse and then you can get a Numeric value from that
https://docs.oracle.com/javase/8/docs/api/java/time/YearMonth.html#parse-java.lang.CharSequence-

I fixed it. Is it better like this?

Yep! Looks better now

Siedlerchr · 2018-02-17T14:44:41Z

src/main/java/org/jabref/model/entry/Month.java

@@ -93,9 +95,31 @@
            int number = Integer.parseInt(value);
            return Month.getMonthByNumber(number);
        } catch (NumberFormatException e) {
-            return Optional.empty();
+            return parseGermanShortMonth(testString);


Better move the method above in the parse method, so the parseGermanMonth will be executed when the others returned empty. Just like it it is deone with the getMonthByShortName

Sorry, I don't know exactly what you mean.

Okay, I will try to explain:
Move the method parseGermanShortMonth before the catch block. and after the getMonthByShortName:

month= Month.parseGerman.... if(month.isPresent) return month;

The end result is the same, but it looks more logical in the whole parse method:

We check if the value is empty

We try to get the normal short month

we try german short month

last try: try into parse it as number

Otherwise from a quick look I would see and think: Okay, if the integer parsing fails it implies it is automatically a German month. And that might not be the case. In reality the integer parsing maybe failed because we entered 9999

Thanks for the detailed explanation. I adjusted it.

koppor

Minor issues regarding formatting an parsing of Maerz.

koppor · 2018-02-18T13:36:02Z

src/main/java/org/jabref/model/entry/Month.java

+	 *            a String that represents a month in German form
+	 * @return the corresponding month instance, empty if input is not in German
+	 *         form
+	 * @author Johannes Pre�mar


Please do not put @author tags. We acknowledge our authors in AUTHORS. See https://github.com/JabRef/jabref/blob/master/CONTRIBUTING.md#author-credits. Please ensure that your git configuration user.name and user.email is correct.

Okey I understand, but I have done that for PE1718, but the worksheet said so for authorize. I deleted it and will put my name directly in the Pull Request.

koppor · 2018-02-18T13:36:21Z

src/main/java/org/jabref/model/entry/Month.java

+	 */
+	private static Optional<Month> parseGermanShortMonth(String value) {
+		try {
+		YearMonth yearMonth = YearMonth.parse("1969-" + value, DateTimeFormatter.ofPattern("yyyy-MMM", Locale.GERMAN));


The indent seems to be wrong here. Use auto-format of your IDE. Ctrl+Alt+L in IntelliJ.

The idea of #3536 was to parse all available languages. Also Spanish, Greek, etc. Therefore, all available locales were fetched at https://github.com/JabRef/jabref/pull/3536/files#diff-3c5fdac594bc2e649536f5ecdcb481ccR122 and iterated on. Is this possible here, too - or does it clutter the code?

After more thinking, I would say, we do not iterate on all locales and keep German only, because only the literature list by http://www2.informatik.uni-stuttgart.de/zdi/buecherei/NCSTRL_listings/FAK/index.html causes these issues and I am not aware of any other systems.

I use Eclipse, hopefully Ctrl + Shift + F will have the same result.

Eiswindyeti · 2018-02-19T07:42:52Z

src/test/java/org/jabref/model/entry/MonthTest.java

@@ -100,4 +100,36 @@ public void parseReturnsEmptyOptionalForInvalidInput() {
    public void parseReturnsEmptyOptionalForEmptyInput() {
        assertEquals(Optional.empty(), Month.parse(""));
    }
+
+    @Test


I adjusted it.

koppor · 2018-02-18T13:39:19Z

src/test/java/org/jabref/model/entry/MonthTest.java

+	public void parseCorrectlyByFullNameGerman() {
+		assertEquals(Optional.of(Month.JANUARY), Month.parse("Januar"));
+		assertEquals(Optional.of(Month.FEBRUARY), Month.parse("Februar"));
+		assertEquals(Optional.of(Month.MARCH), Month.parse("M�rz"));


Done: ~~Please also add a test for Maerz, which is returned by the Stuttgart System: http://www2.informatik.uni-stuttgart.de/zdi/buecherei/NCSTRL_listings/FAK/index.html~~

I also put in an if-statement for checking for Maerz in the method parseGermanShortMonth, because I was not sure if the Locale checks for this.

Eiswindyeti · 2018-02-19T07:42:52Z

src/test/java/org/jabref/model/entry/MonthTest.java

@@ -100,4 +100,36 @@ public void parseReturnsEmptyOptionalForInvalidInput() {
    public void parseReturnsEmptyOptionalForEmptyInput() {
        assertEquals(Optional.empty(), Month.parse(""));
    }
+
+    @Test


I adjusted it.

koppor · 2018-02-19T07:51:25Z

src/main/java/org/jabref/model/entry/Month.java

@@ -88,33 +88,41 @@
        if (testString.length() > 3) {
            testString = testString.substring(0, 3);
        }
-        Optional<Month> month = Month.getMonthByShortName(testString);


The indent is wrong here. I have no clue, what is going wrong on your side. Do you use tabs instead of spaces?

I don't know what went wrong. I used the auto formatter of Eclipse, so it could be the tabs. Now it looks better after replacing the tabs with spaces by hand. I should definetly change the IDE if this is a common problem.

Siedlerchr · 2018-02-19T08:19:40Z

src/main/java/org/jabref/model/entry/Month.java

+	 */
+	private static Optional<Month> parseGermanShortMonth(String value) {
+		if ("Mae".equals(value)) {
+			return Month.getMonthByNumber(3);


To test whether the if is needed, uncomment it and execute the unit test.
If the test is green, you won't need it....

Regarding the code style format, did you execute gradlew eclipse? It should import the correct style settings

The test is then red. So I leave the if-statement in there?

stefan-kolb · 2018-02-19T09:57:45Z

src/main/java/org/jabref/model/entry/Month.java

+     *         form
+     */
+	private static Optional<Month> parseGermanShortMonth(String value) {
+        if ("Mae".equals(value)) {


What if it is "mae" in lowercase?

The implementation should use equalsIgnoreCase.

Please fix indent.

stefan-kolb · 2018-02-19T09:58:06Z

src/test/java/org/jabref/model/entry/MonthTest.java

+
+    @Test
+    public void parseCorrectlyByShortNameGerman() {
+        assertEquals(Optional.of(Month.JANUARY), Month.parse("Jan"));


Also add tests for lowercase?

The German short forms are in uppercase. So what should be tested?

Well, then just copy the test cases to a new method and then just call month. Parse with the lowercase name
And execute the tests then

Tests were added.

koppor

Small things remain.

koppor · 2018-02-19T21:45:07Z

src/main/java/org/jabref/model/entry/Month.java

@@ -85,9 +89,13 @@
            testString = testString.substring(0, 3);
        }
        Optional<Month> month = Month.getMonthByShortName(testString);
+        Optional<Month> monthGerman = Month.parseGermanShortMonth(testString);


Please move that after line 95. the German month only has to be determined if there is no English month.

koppor · 2018-02-19T21:45:31Z

src/main/java/org/jabref/model/entry/Month.java

+     *         form
+     */
+	private static Optional<Month> parseGermanShortMonth(String value) {
+        if ("Mae".equals(value)) {


Please fix indent.

koppor · 2018-02-20T12:11:42Z

src/main/java/org/jabref/model/entry/Month.java

@@ -115,6 +117,9 @@
     *         form
     */
 	private static Optional<Month> parseGermanShortMonth(String value) {
+        // support for lowercase German month
+        value = value.substring(0,1).toUpperCase() + value.substring(1);


Please remove this and the line above. Just use equalsIgnoreCase in line 123.

Now no German months are accepted with lowercase, just mae.

I overlooked this case. I think, you have to change the if then to following

if (("Mae".equalsIgnoreCase(value)) || ("Maerz".equalsIgnoreCase(value))) {

Please also ensure that a test case for Maerz is in there. If it was already, then I don't get why that code still worked.

The dateformatter is case-sensitive by default. (Adapting) the following code should work:

DateTimeFormatter formatter = new DateTimeFormatterBuilder() .parseCaseInsensitive() .appendPattern("yyyy-MMM") .toFormatter(Locale.US);

koppor

Discussion regarding "Maerz" and "Mae" ongoing.

koppor · 2018-02-20T13:50:23Z

src/main/java/org/jabref/model/entry/Month.java

@@ -115,6 +117,9 @@
     *         form
     */
 	private static Optional<Month> parseGermanShortMonth(String value) {
+        // support for lowercase German month
+        value = value.substring(0,1).toUpperCase() + value.substring(1);


I overlooked this case. I think, you have to change the if then to following

if (("Mae".equalsIgnoreCase(value)) || ("Maerz".equalsIgnoreCase(value))) {

Please also ensure that a test case for Maerz is in there. If it was already, then I don't get why that code still worked.

koppor · 2018-02-20T13:51:23Z

Please merge JabRefs' branch master (should be available at upstream/master) to resolve conflicts in CHANGELOG.md

lenhard · 2018-02-21T11:02:17Z

The tests are still failing. From looking at the code, I think that "Mär" is not parsed correctly. Could you please investigate this?

Once all tests are passing on travis, I think we can merge this.

koppor · 2018-03-09T19:01:11Z

I'll merge into wip, finish the PR and merge it into master.

koppor · 2018-03-09T19:46:23Z

I fixed the issues at #3742

The java files had the wrong encoding (UTF8 was not used)
The implementation for "mär" and "märz" was missing.

Eiswindyeti and others added 2 commits February 17, 2018 14:36

Add parser for German months (#3536)

19223c8

Update Month.java

a1daf61

Siedlerchr reviewed Feb 17, 2018

View reviewed changes

Update CHANGELOG.md

94fb247

Siedlerchr reviewed Feb 17, 2018

View reviewed changes

Eiswindyeti and others added 2 commits February 17, 2018 17:11

3536: Changed to java 8 Date & Time Api

b5dabe7

Update Month.java

ef98eb9

koppor mentioned this pull request Feb 18, 2018

[WIP] Add support for German months at the normalizer functionality #3536

Closed

6 tasks

koppor requested changes Feb 18, 2018

View reviewed changes

koppor added the PE1718 label Feb 18, 2018

Eiswindyeti added 2 commits February 19, 2018 08:17

adjust return order of parse and correct indent

87b6273

fix some more indent and adjust String comparison

5b8f3f7

Eiswindyeti commented Feb 19, 2018

View reviewed changes

koppor reviewed Feb 19, 2018

View reviewed changes

Correct indent

bf88e94

Siedlerchr reviewed Feb 19, 2018

View reviewed changes

Eiswindyeti added 2 commits February 19, 2018 09:23

Correct indent

7cf796c

Correct indent

7c585ba

stefan-kolb reviewed Feb 19, 2018

View reviewed changes

koppor requested changes Feb 19, 2018

View reviewed changes

Eiswindyeti added 2 commits February 20, 2018 11:58

Add support fo lowercase German month

d84ec09

Correct indent

52b153c

koppor reviewed Feb 20, 2018

View reviewed changes

Add equalsIgnoreCase

5d56874

tobiasdiez approved these changes Feb 20, 2018

View reviewed changes

koppor requested changes Feb 20, 2018

View reviewed changes

Eiswindyeti and others added 2 commits February 20, 2018 15:55

Change if-Statement

73a7ee7

Merge branch 'master' into add-parser-for-German-months-issue-3536

0124fe1

Siedlerchr approved these changes Feb 20, 2018

View reviewed changes

Eiswindyeti closed this Feb 21, 2018

Eiswindyeti deleted the add-parser-for-German-months-issue-3536 branch February 21, 2018 13:05

Eiswindyeti restored the add-parser-for-German-months-issue-3536 branch February 21, 2018 13:07

Eiswindyeti deleted the add-parser-for-German-months-issue-3536 branch February 21, 2018 13:07

Eiswindyeti restored the add-parser-for-German-months-issue-3536 branch February 21, 2018 13:10

Eiswindyeti reopened this Feb 21, 2018

koppor changed the base branch from master to wip March 9, 2018 18:57

koppor merged commit 39f495d into JabRef:wip Mar 9, 2018

koppor removed the PE1718 label Mar 9, 2018

Add parser for German months (#3536) #3734

Add parser for German months (#3536) #3734

Conversation

Eiswindyeti commented Feb 17, 2018 • 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

Choose a reason for hiding this comment

koppor 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

koppor Feb 18, 2018 • 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

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

koppor 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

koppor Feb 20, 2018 • 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

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor commented Feb 20, 2018

lenhard commented Feb 21, 2018

koppor commented Mar 9, 2018

koppor commented Mar 9, 2018

Eiswindyeti commented Feb 17, 2018 •

edited

Loading

koppor Feb 18, 2018 •

edited

Loading

koppor Feb 20, 2018 •

edited

Loading