Introduce formatter to remove word-enclosing braces #11253

rohit-garga · 2024-04-26T13:05:31Z

Mandatory checks

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

rohit-garga · 2024-04-26T13:57:47Z

It's not clear to me why fetcher tests are failing, they failed even after I reverted my changes. Can you help please @koppor

Siedlerchr · 2024-04-26T13:59:24Z

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

koppor · 2024-04-26T15:09:42Z

@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 architecture and model.

Does not work for gui. There, you need to choose guiTest.

rohit-garga · 2024-04-26T16:31:42Z

Understood, thanks! Let me know when you get chance to review :)

…f }} at the end

koppor

Some small comment.

I fixed other things locally, but I don't have push rights to your brunch. Thus, I will submit a PR.

koppor · 2024-04-29T07:52:28Z

src/main/java/org/jabref/logic/formatter/casechanger/ProtectTermsFormatter.java

 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}


No import for JavaDoc classes please

Not fixed, but checkstyle did not complain. Thus, I let it go.

koppor · 2024-04-29T08:04:32Z

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

Small code fixes

koppor · 2024-05-01T08:10:10Z

src/main/java/org/jabref/logic/formatter/bibtexfields/RemoveEnclosingBracesFormatter.java

@@ -15,7 +15,7 @@ public String getName() {

    @Override
    public String getKey() {
-        return "remove_braces";


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....

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

rohit-garga · 2024-05-04T10:50:40Z

Hi @koppor ,
I need some help-

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

PersonNameSuggestionProviderTest.completeLowercaseBeginningOfNameReturnsName
Expected :[Author{givenName='Vassilis', givenNameAbbreviated='V.', namePrefix='', familyName='Kostakos', nameSuffix=''}]
Actual :[Author{givenName='Vassilis', givenNameAbbreviated='V.', namePrefix='null', familyName='Kostakos', nameSuffix='null'}]

2.)

FormatterTest.formatOfNullThrowsException

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
1.) Return null whenever we get an empty string/ null.
2.) We add a try-catch in the author constructor, so we can catch null pointer exceptions and continue.

koppor · 2024-05-04T11:20:29Z

@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

Thank you for continuing this. Minor comments remain.

koppor · 2024-05-07T09:53:51Z

src/main/java/org/jabref/logic/formatter/casechanger/ProtectTermsFormatter.java

 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}


Not fixed, but checkstyle did not complain. Thus, I let it go.

koppor · 2024-05-07T09:54:52Z

src/main/java/org/jabref/model/entry/Author.java

    /**
     * Returns the first name of the author stored in this object ("First").
     *
     * @return first name of the author (may consist of several tokens)
     */
+


Why this empty line here? JavaDoc comments should be as close as possible to the method.

koppor · 2024-05-07T09:57:14Z

src/main/java/org/jabref/model/entry/Author.java

 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();


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)

koppor · 2024-05-13T09:01:10Z

@rohit-garga Thank you for the follow-ups. I think, the null handling is now OK. Someone touching the code later will see. 😅

* 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)

rohit-garga added 3 commits April 26, 2024 07:53

JabRef#11222 fix

9d67d15

checkstyle fixes

07ba27f

fixes for unit tests

5ee6946

koppor added 6 commits April 29, 2024 09:36

Make new RemoveWordEnclosingAndOuterEnclosingBracesFormatter available

e5a0a9a

Add more braces to the example input

2ee4699

Keep original string when formatting empty string

882d31b

Fix RemoveWordEnclosingAndOuterEnclosingBracesFormatter in the case o…

3d7fa97

…f }} at the end

Fix constant (and empty line)

b5ba46b

Reorder methods

ac21b4a

koppor requested changes Apr 29, 2024

View reviewed changes

Add CHANGELOG.md entry

087efdf

koppor mentioned this pull request Apr 29, 2024

Small code fixes rohit-garga/jabref#1

Merged

6 tasks

koppor mentioned this pull request Apr 29, 2024

Fix Biodiversity fetcher test results #11257

Merged

6 tasks

rohit-garga and others added 6 commits April 29, 2024 22:06

Merge pull request #1 from koppor/improve-fix-for-issue-11251

518de66

Small code fixes

Merge branch 'JabRef:main' into fix-for-issue-11251

94c425c

markdown issue fix

67fc88f

checkstyle fix

e446efe

duplicate keys issue fix

1013d9a

author class not being initialized fix

27551c4

koppor reviewed May 1, 2024

View reviewed changes

rohit-garga added 3 commits May 1, 2024 15:38

reverting key name change for old formatter

aa0ebf3

reverting commit- Keep original string when formatting empty string s…

62cfea1

…ince it causes unit tests to fail

minor fixes

afae4cd

rohit-garga and others added 4 commits May 7, 2024 07:51

minor fixes

6083b96

Merge branch 'main' into fix-for-issue-11251

3e2d6bc

minor fixes

9ef10c8

fix: rewrite

3ee101d

rohit-garga requested a review from koppor May 7, 2024 03:11

koppor requested changes May 7, 2024

View reviewed changes

formatting fix

3f85ac1

rohit-garga requested a review from koppor May 9, 2024 06:10

koppor approved these changes May 13, 2024

View reviewed changes

koppor added this pull request to the merge queue May 13, 2024

Merged via the queue into JabRef:main with commit 0eab5dc May 13, 2024
21 of 22 checks passed

rohit-garga deleted the fix-for-issue-11251 branch May 13, 2024 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce formatter to remove word-enclosing braces #11253

Introduce formatter to remove word-enclosing braces #11253

rohit-garga commented Apr 26, 2024

rohit-garga commented Apr 26, 2024

Siedlerchr commented Apr 26, 2024

koppor commented Apr 26, 2024

rohit-garga commented Apr 26, 2024

koppor left a comment

koppor Apr 29, 2024

koppor May 7, 2024

koppor commented Apr 29, 2024

koppor May 1, 2024

rohit-garga May 1, 2024

rohit-garga commented May 4, 2024

koppor commented May 4, 2024

koppor left a comment

koppor May 7, 2024

koppor May 7, 2024

koppor May 7, 2024

koppor commented May 13, 2024

Introduce formatter to remove word-enclosing braces #11253

Introduce formatter to remove word-enclosing braces #11253

Conversation

rohit-garga commented Apr 26, 2024

Mandatory checks

rohit-garga commented Apr 26, 2024

Siedlerchr commented Apr 26, 2024

koppor commented Apr 26, 2024

rohit-garga commented Apr 26, 2024

koppor left a comment

Choose a reason for hiding this comment

koppor Apr 29, 2024

Choose a reason for hiding this comment

koppor May 7, 2024

Choose a reason for hiding this comment

koppor commented Apr 29, 2024

koppor May 1, 2024

Choose a reason for hiding this comment

rohit-garga May 1, 2024

Choose a reason for hiding this comment

rohit-garga commented May 4, 2024

koppor commented May 4, 2024

koppor left a comment

Choose a reason for hiding this comment

koppor May 7, 2024

Choose a reason for hiding this comment

koppor May 7, 2024

Choose a reason for hiding this comment

koppor May 7, 2024

Choose a reason for hiding this comment

koppor commented May 13, 2024