Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add multi-sentence title formatting to sentence case and title case #6872

Merged
merged 7 commits into from
Oct 3, 2020
Merged

Add multi-sentence title formatting to sentence case and title case #6872

merged 7 commits into from
Oct 3, 2020

Conversation

tmrd993
Copy link
Contributor

@tmrd993 tmrd993 commented Sep 4, 2020

Fixes #6759

Added support for multiple-sentence title formatting for the "Sentence Case" and "Title Case" options by using a regular expression to first split the input into sentences, and afterwards processing those sentences individually. A RegEx might not be the best choice for natural language processing but since most titles are only 1 sentence long, the proposed RegEx should cover the vast majority of cases. Also added additional test cases for Sentence Case and Title Case formatting. The old tests are assuming one sentence titles but are still applicable.

  • Change in CHANGELOG.md described (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome to JabRef development! I am very happy to see an ADR. Regarding the code, I just have some minor comments.

src/main/java/org/jabref/model/strings/StringUtil.java Outdated Show resolved Hide resolved
@@ -25,6 +25,36 @@ public void test() {
assertEquals("Upper {N}ot first", formatter.format("upper {N}OT FIRST"));
}

@Test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you are on it, I propose to rewrite to parameterized tests:

package org.jabref.logic.formatter.casechanger;

import java.util.stream.Stream;

import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import static org.junit.jupiter.api.Assertions.assertEquals;

/**
 * Tests in addition to the general tests from {@link org.jabref.logic.formatter.FormatterTest}
 */
public class SentenceCaseFormatterTest {

    private SentenceCaseFormatter formatter = new SentenceCaseFormatter();

    private static Stream<Arguments> testData() {
        return Stream.of(
                Arguments.of("Upper first", "upper First"),
                Arguments.of("Upper first", "uPPER FIRST"),
                Arguments.of("Upper {NOT} first", "upper {NOT} FIRST"),
                Arguments.of("Upper {N}ot first", "upper {N}OT FIRST"),
                Arguments.of("Whose music? A sociology of musical language",
                        "Whose music? a sociology of musical language"),
                Arguments.of("Bibliographic software. A comparison.",
                        "bibliographic software. a comparison."),
                Arguments.of("England’s monitor; The history of the separation",
                        "England’s Monitor; the History of the Separation"),
                Arguments.of("Dr. schultz: a dentist turned bounty hunter.",
                        "Dr. schultz: a dentist turned bounty hunter."),
                Arguments.of("Example case. {EXCLUDED SENTENCE.}",
                        "Example case. {EXCLUDED SENTENCE.}"),
                Arguments.of("I have {Aa} dream", new SentenceCaseFormatter().getExampleInput())
        );
    }

    @ParameterizedTest
    @MethodSource("testData")
    public void test(String expected, String input) {
        assertEquals(expected, formatter.format(input));
    }
}

OK, the descriptions are lost. Maybe, the description could be added as first paramter to the test method.

@koppor koppor added the status: changes required Pull requests that are not yet complete label Sep 20, 2020
@koppor
Copy link
Member

koppor commented Sep 23, 2020

Thank you for working on it - could you fix checkstyle? 😇

[ant:checkstyle] [ERROR] /home/runner/work/jabref/jabref/src/main/java/org/jabref/logic/formatter/casechanger/TitleCaseFormatter.java:37: There is more than 1 empty line one after another. [EmptyLineSeparator]

Even though there are many failing tests, we should at least try to keep the essential tests green. "Checkstyle" and "Unit tests" are essential. "Unit tests" go through; just checkstyle is it.

@tmrd993
Copy link
Contributor Author

tmrd993 commented Sep 23, 2020

Done. How can I test for checkstyle issues?

I am using gradlew check as explained in the doc but it only does unit tests.

@Siedlerchr
Copy link
Member

If you use intellij you can install the checkstyle plug-in and also import the jabref code style for the formatter.
Otherwise just run gradlew checkstyle.
You also see the failing tests if you click on the checkstyle CI test

assertEquals(expected, formatter.format(input));
}

/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why these tests are commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are redundant as I already added them as parameterized tests but I didn't know if I should delete them yet since they contain the descriptions of the tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then just remove them. I think it's clear from the arguments what is tested

@koppor koppor merged commit 9943074 into JabRef:master Oct 3, 2020
@Siedlerchr
Copy link
Member

@tmrd993 Thanks again for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: changes required Pull requests that are not yet complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Sentence case" and "Title case" not enforcing a capital after a period
3 participants