-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Formatter refactoring #3919
Formatter refactoring #3919
Conversation
if (formatter.isPresent()) { | ||
return formatter; | ||
Optional<Formatter> formatter = getAll().stream().filter(f -> f.getKey().equals(modifier)).findAny(); | ||
if (formatter.isPresent()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
new NormalizePagesFormatter(), | ||
new OrdinalsToSuperscriptFormatter(), | ||
new ProtectTermsFormatter(protectedTermsLoader), | ||
new RegexFormatter("(\" \",\"-\")"), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
} | ||
|
||
@ParameterizedTest | ||
@MethodSource("getFormatters") | ||
public void getNameReturnsNotNull(Formatter formatter) { | ||
void getNameReturnsNotNull(Formatter formatter) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
import org.junit.jupiter.api.Test; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
||
/** | ||
* Tests in addition to the general tests from {@link org.jabref.logic.formatter.FormatterTest} | ||
*/ | ||
public class RegexFormatterTest { | ||
class RegexFormatterTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
Refactor the regular expression-based formatter. Previously, the regex was sent through a static
setRegex
method, now it gets passed as a normal constructor argument.