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

Fixes bugs in the regex cite key pattern modifier #6893

Merged
merged 10 commits into from
Sep 21, 2020
Merged

Fixes bugs in the regex cite key pattern modifier #6893

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1c15665
Add regex modifier test cases
k3KAW8Pnf7mkmdSMPHz27 Sep 8, 2020
6ee994a
Add test case for empty argument
k3KAW8Pnf7mkmdSMPHz27 Sep 8, 2020
4cfaf41
Fix handling of constructor argument
k3KAW8Pnf7mkmdSMPHz27 Sep 9, 2020
3f20a97
Fix JavaDoc
k3KAW8Pnf7mkmdSMPHz27 Sep 9, 2020
bc9e383
Add error handling
k3KAW8Pnf7mkmdSMPHz27 Sep 9, 2020
0426d09
Fix speeding up matching of regex
k3KAW8Pnf7mkmdSMPHz27 Sep 9, 2020
a491923
Fix logger string
k3KAW8Pnf7mkmdSMPHz27 Sep 21, 2020
721a50c
Fix logger string
k3KAW8Pnf7mkmdSMPHz27 Sep 21, 2020
c11f449
Fix readability
k3KAW8Pnf7mkmdSMPHz27 Sep 21, 2020
e4674a7
Add test case for regex with syntax error
k3KAW8Pnf7mkmdSMPHz27 Sep 21, 2020
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
33 changes: 21 additions & 12 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 @@ -18,28 +18,37 @@ public class RegexFormatter extends Formatter {
// \\ is required to have the { interpreted as character
// ? is required to disable the aggressive match
private static final Pattern PATTERN_ENCLOSED_IN_CURLY_BRACES = Pattern.compile("(\\{.*?})");
private static final String REGEX_CAPTURING_GROUP = "regex";
private static final String REPLACEMENT_CAPTURING_GROUP = "replacement";
/**
* Matches a valid argument to the constructor. Two capturing groups are used to parse the {@link
* RegexFormatter#regex} and {@link RegexFormatter#replacement} used in {@link RegexFormatter#format(String)}
*/
private static final Pattern CONSTRUCTOR_ARGUMENT = Pattern.compile(
"^\\(\"(?<" + REGEX_CAPTURING_GROUP + ">.*?)\" *?, *?\"(?<" + REPLACEMENT_CAPTURING_GROUP + ">.*?)\"\\)$");
k3KAW8Pnf7mkmdSMPHz27 marked this conversation as resolved.
Show resolved Hide resolved
// Magic arbitrary unicode char, which will never appear in bibtex files
private static final String PLACEHOLDER_FOR_PROTECTED_GROUP = Character.toString('\u0A14');
private static final String PLACEHOLDER_FOR_OPENING_CURLY_BRACE = Character.toString('\u0A15');
private static final String PLACEHOLDER_FOR_CLOSING_CURLY_BRACE = Character.toString('\u0A16');
private static final String QUOTE_AND_OPENING_BRACE = "\"(";
private static final int LENGTH_OF_QUOTE_AND_OPENING_BRACE = QUOTE_AND_OPENING_BRACE.length();
private static final String CLOSING_BRACE_AND_QUOTE = ")\"";
private static final int LENGTH_OF_CLOSING_BRACE_AND_QUOTE = CLOSING_BRACE_AND_QUOTE.length();
private static String regex;
private String replacement;
private final String regex;
private final 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];
Objects.requireNonNull(input);
input = input.trim();
Matcher constructorArgument = CONSTRUCTOR_ARGUMENT.matcher(input);
if (constructorArgument.matches()) {
regex = constructorArgument.group(REGEX_CAPTURING_GROUP);
replacement = constructorArgument.group(REPLACEMENT_CAPTURING_GROUP);
} else {
regex = null;
replacement = null;
}
}

@Override
Expand Down Expand Up @@ -71,7 +80,7 @@ private String replaceHonoringProtectedGroups(final String input) {
@Override
public String format(final String input) {
Objects.requireNonNull(input);
if (regex == null) {
if (regex == null || replacement == null) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you switch to Optional<String> as datatype for regex and replacement? JabRef wants to get rid off null.

Copy link
Member Author

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 Sep 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Not easily without code-style complaints. regex and replacement are instance fields (private final String).

I believe the choice of using instance fields is to inherit the method logic.cleanup.Formatter#format(String value), which most modifiers do, while still allowing arguments.

The behavior of most code in the citation key patterns is to either return an empty string or an unmodified string on invalid usage. Hence throwing an exception during construction would require code outside of RegexFormatter to deal with that circumstance, which I'd like to avoid due to separation of concerns.

I'd guess the alternative is to create an interface extending logic.cleanup.Formatter with format(String value, String ... args).

I have avoided taking these types of decisions as I am fairly new to Java and JabRef, but I can look into other alternatives.

return input;
}

Expand Down
27 changes: 24 additions & 3 deletions src/test/java/org/jabref/logic/formatter/bibtexfields/RegexFormatterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@

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

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

private RegexFormatter formatter;
Expand Down Expand Up @@ -52,4 +49,28 @@ void formatExample() {
formatter = new RegexFormatter("(\" \",\"-\")");
assertEquals("Please-replace-the-spaces", formatter.format(formatter.getExampleInput()));
}

@Test
void formatCanRemoveMatchesWithEmptyReplacement() {
formatter = new RegexFormatter("(\"[A-Z]\",\"\")");
assertEquals("abc", formatter.format("AaBbCc"));
}

@Test
void constructorWithInvalidConstructorArgumentReturnUnchangedString() {
formatter = new RegexFormatter("(\"\",\"\"");
assertEquals("AaBbCc", formatter.format("AaBbCc"));
}

@Test
void constructorWithEmptyStringArgumentReturnUnchangedString() {
formatter = new RegexFormatter("");
assertEquals("AaBbCc", formatter.format("AaBbCc"));
}

@Test
void constructorAllowsSpacesBetweenQuotes() {
formatter = new RegexFormatter("(\"[A-Z]\", \"\")");
assertEquals("abc", formatter.format("AaBbCc"));
}
}