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.

Sign up for GitHub

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

Formatter refactoring #3919

Merged
merged 4 commits into from
Apr 4, 2018
Merged

Formatter refactoring #3919

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
84da698
Refactor RegEx Formatter
tobiasdiez Apr 3, 2018
5014dcf
Fix formatting
tobiasdiez Apr 3, 2018
7eafad2
Reorder return statements in `getFormatterForModifier`
tobiasdiez Apr 3, 2018
f541d05
Fix order in RegexFormatter
tobiasdiez Apr 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 40 additions & 60 deletions src/main/java/org/jabref/logic/formatter/Formatters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,83 +31,63 @@

public class Formatters {

private static final List<Formatter> CONVERTERS = Arrays.asList(
new HtmlToLatexFormatter(),
new HtmlToUnicodeFormatter(),
new LatexToUnicodeFormatter(),
new UnicodeToLatexFormatter()
);

private static final List<Formatter> CASE_CHANGERS = Arrays.asList(
new CapitalizeFormatter(),
new LowerCaseFormatter(),
new SentenceCaseFormatter(),
new TitleCaseFormatter(),
new UpperCaseFormatter()
);

private static final List<Formatter> OTHERS = Arrays.asList(
new ClearFormatter(),
new LatexCleanupFormatter(),
new MinifyNameListFormatter(),
new NormalizeDateFormatter(),
new NormalizeMonthFormatter(),
new NormalizeNamesFormatter(),
new NormalizePagesFormatter(),
new OrdinalsToSuperscriptFormatter(),
new RegexFormatter(),
new RemoveBracesFormatter(),
new UnitsToLatexFormatter(),
new EscapeUnderscoresFormatter()
);

private static final String REGEX = "regex";

private static final int LENGTH_OF_REGEX_PREFIX = REGEX.length();

private Formatters() {
}

public static final List<Formatter> getConverters() {
List<Formatter> converters = new ArrayList<>();
converters.addAll(CONVERTERS);
return converters;
public static List<Formatter> getConverters() {
return Arrays.asList(
new HtmlToLatexFormatter(),
new HtmlToUnicodeFormatter(),
new LatexToUnicodeFormatter(),
new UnicodeToLatexFormatter()
);
}

public static final List<Formatter> getCaseChangers() {
List<Formatter> caseChangers = new ArrayList<>();
caseChangers.addAll(CASE_CHANGERS);
return caseChangers;
public static List<Formatter> getCaseChangers() {
return Arrays.asList(
new CapitalizeFormatter(),
new LowerCaseFormatter(),
new SentenceCaseFormatter(),
new TitleCaseFormatter(),
new UpperCaseFormatter()
);
}

public static final List<Formatter> getOthers() {
List<Formatter> others = new ArrayList<>();
others.addAll(OTHERS);
return others;
public static List<Formatter> getOthers() {
return Arrays.asList(
new ClearFormatter(),
new LatexCleanupFormatter(),
new MinifyNameListFormatter(),
new NormalizeDateFormatter(),
new NormalizeMonthFormatter(),
new NormalizeNamesFormatter(),
new NormalizePagesFormatter(),
new OrdinalsToSuperscriptFormatter(),
new RemoveBracesFormatter(),
new UnitsToLatexFormatter(),
new EscapeUnderscoresFormatter()
);
}

public static final List<Formatter> getAll() {
public static List<Formatter> getAll() {
List<Formatter> all = new ArrayList<>();
all.addAll(CONVERTERS);
all.addAll(CASE_CHANGERS);
all.addAll(OTHERS);
all.addAll(getConverters());
all.addAll(getCaseChangers());
all.addAll(getOthers());
return all;
}

public static Optional<Formatter> getFormatterForModifier(String modifier) {
Objects.requireNonNull(modifier);
Optional<Formatter> formatter;
List<Formatter> all = getAll();

if (modifier.matches("regex.*")) {
String regex = modifier.substring(LENGTH_OF_REGEX_PREFIX);
RegexFormatter.setRegex(regex);
formatter = all.stream().filter(f -> f.getKey().equals("regex")).findAny();
if (modifier.startsWith(RegexFormatter.KEY)) {
String regex = modifier.substring(RegexFormatter.KEY.length());
return Optional.of(new RegexFormatter(regex));
} else {
formatter = all.stream().filter(f -> f.getKey().equals(modifier)).findAny();
}
if (formatter.isPresent()) {
return formatter;
Optional<Formatter> formatter = getAll().stream().filter(f -> f.getKey().equals(modifier)).findAny();
if (formatter.isPresent()) {
Copy link
Member

Choose a reason for hiding this comment

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

That does not make any sense does it?
The method returns an optional. So you can directly return formatter.

Copy link
Member Author

Choose a reason for hiding this comment

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

If no formatter is found, then we still want to try the switch at the end. I now reordered the code a bit and it should be clearer now. Good point.

return formatter;
}
}
switch (modifier) {
case "lower":
Expand Down
33 changes: 19 additions & 14 deletions src/main/java/org/jabref/logic/formatter/bibtexfields/RegexFormatter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,23 @@ public class RegexFormatter implements Formatter {
private static final String CLOSING_BRACE_AND_QUOTE = ")\"";

private static final int LENGTH_OF_CLOSING_BRACE_AND_QUOTE = CLOSING_BRACE_AND_QUOTE.length();

// stores the regex set by setRegex
private static String[] regex;
public static final String KEY = "regex";

private String regex;
private String replacement;

/**
* Constructs a new regular expression-based formatter with the given RegEx.
*
* @param input the regular expressions for matching and replacing given in the form {@code (<regex>, <replace>)}.
*/
public RegexFormatter(String input) {
// formatting is like ("exp1","exp2"), we want to first remove (" and ")
String rexToSet = input.substring(LENGTH_OF_QUOTE_AND_OPENING_BRACE, input.length() - LENGTH_OF_CLOSING_BRACE_AND_QUOTE);
String[] parts = rexToSet.split("\",\"");
regex = parts[0];
replacement = parts[1];
}

@Override
public String getName() {
Expand All @@ -45,7 +59,7 @@ public String getName() {

@Override
public String getKey() {
return "regex";
return KEY;
}

private String replaceHonoringProtectedGroups(final String input) {
Expand All @@ -56,7 +70,7 @@ private String replaceHonoringProtectedGroups(final String input) {
replaced.add(matcher.group(1));
}
String workingString = matcher.replaceAll(PLACEHOLDER_FOR_PROTECTED_GROUP);
workingString = workingString.replaceAll(RegexFormatter.regex[0], RegexFormatter.regex[1]);
workingString = workingString.replaceAll(regex, replacement);

for (String r : replaced) {
workingString = workingString.replaceFirst(PLACEHOLDER_FOR_PROTECTED_GROUP, r);
Expand Down Expand Up @@ -93,13 +107,4 @@ public String getDescription() {
public String getExampleInput() {
return "Please replace the spaces";
}

public static void setRegex(String rex) {
// formatting is like ("exp1","exp2"), we want to remove (" and ")
String rexToSet = rex;
rexToSet = rexToSet.substring(LENGTH_OF_QUOTE_AND_OPENING_BRACE, rexToSet.length() - LENGTH_OF_CLOSING_BRACE_AND_QUOTE);
String[] parts = rexToSet.split("\",\"");
regex = parts;
}

}
85 changes: 42 additions & 43 deletions src/test/java/org/jabref/logic/formatter/FormatterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,102 +38,101 @@
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;

public class FormatterTest {
class FormatterTest {

private static ProtectedTermsLoader protectedTermsLoader;

@BeforeAll
public static void setUp() {
static void setUp() {
protectedTermsLoader = new ProtectedTermsLoader(
new ProtectedTermsPreferences(ProtectedTermsLoader.getInternalLists(), Collections.emptyList(),
Collections.emptyList(), Collections.emptyList()));

}

private static Stream<Formatter> getFormatters() {
// all classes implementing {@link net.sf.jabref.model.cleanup.Formatter}
// sorted alphabetically
// Alternative: Use reflection - https://github.com/ronmamo/reflections
// @formatter:off
return Stream.of(
new CapitalizeFormatter(),
new ClearFormatter(),
new HtmlToLatexFormatter(),
new HtmlToUnicodeFormatter(),
new IdentityFormatter(),
new LatexCleanupFormatter(),
new LatexToUnicodeFormatter(),
new LowerCaseFormatter(),
new MinifyNameListFormatter(),
new NormalizeDateFormatter(),
new NormalizeMonthFormatter(),
new NormalizeNamesFormatter(),
new NormalizePagesFormatter(),
new OrdinalsToSuperscriptFormatter(),
new ProtectTermsFormatter(protectedTermsLoader),
new RegexFormatter("(\" \",\"-\")"),
Copy link
Member

Choose a reason for hiding this comment

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

I don't want to do nitpkicking but that Regex Formatter indentation is clear off

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, no idea why there were these @formatter comments that prevented an automatic code formatting...

new RemoveBracesFormatter(),
new SentenceCaseFormatter(),
new TitleCaseFormatter(),
new UnicodeToLatexFormatter(),
new UnitsToLatexFormatter(),
new UpperCaseFormatter());

// @formatter:on
}

@ParameterizedTest
@MethodSource("getFormatters")
public void getNameReturnsNotNull(Formatter formatter) {
void getNameReturnsNotNull(Formatter formatter) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do you remove the public?
Codacy or whatever will complain that you should always use explicit scoping modifiers. I see no harm in using public here. I don't know if this even has to be public to be accessible from gradle

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the new convention with JUnit 5 that test classes and their methods are package private. IntellJ even complains if they are marked with public.

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay, that was new to me

assertNotNull(formatter.getName());
}

@ParameterizedTest
@MethodSource("getFormatters")
public void getNameReturnsNotEmpty(Formatter formatter) {
void getNameReturnsNotEmpty(Formatter formatter) {
assertNotEquals("", formatter.getName());
}

@ParameterizedTest
@MethodSource("getFormatters")
public void getKeyReturnsNotNull(Formatter formatter) {
void getKeyReturnsNotNull(Formatter formatter) {
assertNotNull(formatter.getKey());
}

@ParameterizedTest
@MethodSource("getFormatters")
public void getKeyReturnsNotEmpty(Formatter formatter) {
void getKeyReturnsNotEmpty(Formatter formatter) {
assertNotEquals("", formatter.getKey());
}

@ParameterizedTest
@MethodSource("getFormatters")
public void formatOfNullThrowsException(Formatter formatter) {
void formatOfNullThrowsException(Formatter formatter) {
assertThrows(NullPointerException.class, () -> formatter.format(null));
}

@ParameterizedTest
@MethodSource("getFormatters")
public void formatOfEmptyStringReturnsEmpty(Formatter formatter) {
void formatOfEmptyStringReturnsEmpty(Formatter formatter) {
assertEquals("", formatter.format(""));
}

@ParameterizedTest
@MethodSource("getFormatters")
public void formatNotReturnsNull(Formatter formatter) {
void formatNotReturnsNull(Formatter formatter) {
assertNotNull(formatter.format("string"));
}

@ParameterizedTest
@MethodSource("getFormatters")
public void getDescriptionAlwaysNonEmpty(Formatter formatter) {
void getDescriptionAlwaysNonEmpty(Formatter formatter) {
assertFalse(formatter.getDescription().isEmpty());
}

@ParameterizedTest
@MethodSource("getFormatters")
public void getExampleInputAlwaysNonEmpty(Formatter formatter) {
void getExampleInputAlwaysNonEmpty(Formatter formatter) {
assertFalse(formatter.getExampleInput().isEmpty());
}

public static Stream<Formatter> getFormatters() {
// all classes implementing {@link net.sf.jabref.model.cleanup.Formatter}
// sorted alphabetically
// Alternative: Use reflection - https://github.com/ronmamo/reflections
// @formatter:off
return Stream.of(
new CapitalizeFormatter(),
new ClearFormatter(),
new HtmlToLatexFormatter(),
new HtmlToUnicodeFormatter(),
new IdentityFormatter(),
new LatexCleanupFormatter(),
new LatexToUnicodeFormatter(),
new LowerCaseFormatter(),
new MinifyNameListFormatter(),
new NormalizeDateFormatter(),
new NormalizeMonthFormatter(),
new NormalizeNamesFormatter(),
new NormalizePagesFormatter(),
new OrdinalsToSuperscriptFormatter(),
new ProtectTermsFormatter(protectedTermsLoader),
new RegexFormatter(),
new RemoveBracesFormatter(),
new SentenceCaseFormatter(),
new TitleCaseFormatter(),
new UnicodeToLatexFormatter(),
new UnitsToLatexFormatter(),
new UpperCaseFormatter());

// @formatter:on
}
}
Loading