Introduce formatter to remove word-enclosing braces#11253
Conversation
|
It's not clear to me why fetcher tests are failing, they failed even after I reverted my changes. Can you help please @koppor |
|
You can ignore the failing fetcher tests. They often have problems due to network errors or changed data. Only relevant if you changed soemthing at the Fetchers itself |
|
@rohit-garga Due to the nature of tests, it is not easy to run all. Sorry for that. Partially running works. E.g., all tests in logic
Then select "test"
Also works for Does not work for |
|
Understood, thanks! Let me know when you get chance to review :) |
koppor
left a comment
There was a problem hiding this comment.
Some small comment.
I fixed other things locally, but I don't have push rights to your brunch. Thus, I will submit a PR.
| * Adds {} brackets around acronyms, month names and countries to preserve their case. | ||
| * | ||
| * Related formatter: {@link org.jabref.logic.formatter.bibtexfields.RemoveBracesFormatter} | ||
| * Related formatter: {@link RemoveEnclosingBracesFormatter} |
There was a problem hiding this comment.
No import for JavaDoc classes please
There was a problem hiding this comment.
Not fixed, but checkstyle did not complain. Thus, I let it go.
|
I have only small comments, but explaining these was more hard than "just doing" it. -- I had no write rights to fix minor issues. Thus, I filed a PR: rohit-garga#1 |
|
|
||
| @Override | ||
| public String getKey() { | ||
| return "remove_braces"; |
There was a problem hiding this comment.
I think, this is the "old" formatter?
Since it is used in .layout files, please don't change the name. Otherwise, we need to add support for alias names....
There was a problem hiding this comment.
Oh, I could not find references to this key anywhere in the code so I made this more descriptive. Reverting this
…ince it causes unit tests to fail
|
Hi @koppor , 1.) We changed the formatter to return the string value instead of null. This does not work, because in the test case, we pass an empty string in the constructor, but a parser will get null not an empty string. For example
2.)
All formatters need to throw a null pointer exception when we run format(null). In our test cases, there are many instances of tests where we pass null in the constructor for Author.java. One easy solution would be |
|
@rohit-garga Step 1 is OK. Step 2: Use an if instead of try/catch, because control flow should not be done using exceptions 😅 |
koppor
left a comment
There was a problem hiding this comment.
Thank you for continuing this. Minor comments remain.
| * Adds {} brackets around acronyms, month names and countries to preserve their case. | ||
| * | ||
| * Related formatter: {@link org.jabref.logic.formatter.bibtexfields.RemoveBracesFormatter} | ||
| * Related formatter: {@link RemoveEnclosingBracesFormatter} |
There was a problem hiding this comment.
Not fixed, but checkstyle did not complain. Thus, I let it go.
| * | ||
| * @return first name of the author (may consist of several tokens) | ||
| */ | ||
|
|
There was a problem hiding this comment.
Why this empty line here? JavaDoc comments should be as close as possible to the method.
| * Example: <code>authors = {Oliver Kopp and others}</code>. This is then appearing as "Oliver Kopp et al.". | ||
| * In the context of BibTeX key generation, this is "Kopp+" (<code>+</code> for "et al.") and not "KO". | ||
| */ | ||
| public static final RemoveWordEnclosingAndOuterEnclosingBracesFormatter FORMATTER = new RemoveWordEnclosingAndOuterEnclosingBracesFormatter(); |
There was a problem hiding this comment.
Very wrong position of the variable. See the javadoc above. This DOES NOT match the new variable, but the OTHERS variable.
Please move the variable two lines down (and separate the next one by an empty line, too)
|
@rohit-garga Thank you for the follow-ups. I think, the |
* upstream/main: Update latex citations status in JavaFx thread (#11302) Remove EnglishStemAnalyzer and use EnglishAnalyzer (#11301) Fix comment (#11299) Try gradle build speedup (#11300) Remove obsolete step (#11295) Bump com.fasterxml.jackson.dataformat:jackson-dataformat-yaml (#11290) Remove outdated pdf indexed files from Lucene index (#11293) Bump src/main/resources/csl-styles from `5338902` to `434df0a` (#11292) Bump org.mockito:mockito-core from 5.11.0 to 5.12.0 (#11291) Bump com.fasterxml.jackson.datatype:jackson-datatype-jsr310 (#11289) Bump com.dlsc.gemsfx:gemsfx from 2.12.0 to 2.16.0 (#11287) Bump org.openrewrite.recipe:rewrite-recipe-bom from 2.9.0 to 2.11.0 (#11288) Introduce formatter to remove word-enclosing braces (#11253) Try parallel tests (#9797) Store preview divider pos in entry editor (#11285)
Closes #11222
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)