-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
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 :) |
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.
Some small comment.
I fixed other things locally, but I don't have push rights to your brunch. Thus, I will submit a PR.
import org.jabref.logic.l10n.Localization; | ||
import org.jabref.logic.protectedterms.ProtectedTermsLoader; | ||
import org.jabref.logic.util.strings.StringLengthComparator; | ||
|
||
/** | ||
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No import for JavaDoc classes please
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.
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 |
@@ -15,7 +15,7 @@ public String getName() { | |||
|
|||
@Override | |||
public String getKey() { | |||
return "remove_braces"; |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😅 |
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.
Thank you for continuing this. Minor comments remain.
import org.jabref.logic.l10n.Localization; | ||
import org.jabref.logic.protectedterms.ProtectedTermsLoader; | ||
import org.jabref.logic.util.strings.StringLengthComparator; | ||
|
||
/** | ||
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not fixed, but checkstyle did not complain. Thus, I let it go.
/** | ||
* Returns the first name of the author stored in this object ("First"). | ||
* | ||
* @return first name of the author (may consist of several tokens) | ||
*/ | ||
|
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 this empty line here? JavaDoc comments should be as close as possible to the method.
public class Author { | ||
|
||
/** | ||
* Object indicating the <code>others</code> author. This is a BibTeX feature mostly rendered in "et al." in LaTeX. | ||
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.md
described in a way that is understandable for the average user (if applicable)