remove Deprecated getText() #11841

leaf-soba · 2024-09-27T08:09:42Z

ref:Refactoring the Usage of Deprecated Java APIs #11674
if this one have done, still remain 23 warnings of this issue.
remove Deprecated getText()

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.

remove Deprecated getText()

koppor · 2024-09-27T08:35:31Z

@leaf-soba you see the failing test? Should be easy to fix 😅

�[0Korg.jabref.logic.layout.LayoutTest�[m

�[0K Test beginConditionals() FAILED

java.lang.NullPointerException: Cannot invoke "org.jabref.model.database.BibDatabase.resolveForStrings(String)" because "database" is null

leaf-soba · 2024-09-27T11:28:50Z

@leaf-soba you see the failing test? Should be easy to fix 😅

�[0Korg.jabref.logic.layout.LayoutTest�[m

�[0K Test beginConditionals() FAILED

java.lang.NullPointerException: Cannot invoke "org.jabref.model.database.BibDatabase.resolveForStrings(String)" because "database" is null

sorry I submit it too hurry forget to run the test this time, I'll fix it.

koppor · 2024-09-29T10:41:59Z

sorry I submit it too hurry forget to run the test this time, I'll fix it.

No worries. We mostly do, too. (Main reason: Not all test succeed on Windows or MacOS locally - refs #9992).

The important thing is to check the GitHub output 30 minutes after a push ^^.

Maybe I should add more unit test based on my change?

koppor

Very small nitpick (because we are aiming for clean code)

One other comment. While thinking through. Did you check your new code with code coverage (by tests)?

koppor · 2024-09-29T15:47:58Z

src/main/java/org/jabref/logic/layout/LayoutEntry.java

@@ -230,6 +230,16 @@ public String doLayout(BibEntry bibtex, BibDatabase database) {
        }
    }

+    private String resolveFieldEntry(BibEntry bibtex, BibDatabase database) {


Rename variable bibtex to `bibEntry - consistency with other code.

koppor · 2024-09-29T15:52:31Z

src/main/java/org/jabref/logic/layout/LayoutEntry.java

@@ -360,7 +368,10 @@ public String doLayout(BibDatabaseContext databaseContext, Charset encoding) {
                throw new UnsupportedOperationException("field and group ends not allowed in begin or end layout");

            case LayoutHelper.IS_OPTION_FIELD:
-                String field = BibDatabase.getText(text, databaseContext.getDatabase());
+                String field = Optional.ofNullable(databaseContext)


I don't think this is necessary - the old code assumed that databaseContext is never null (otherwise an NPE would be there)

koppor · 2024-09-29T15:52:40Z

src/main/java/org/jabref/logic/layout/LayoutEntry.java

@@ -360,7 +368,10 @@ public String doLayout(BibDatabaseContext databaseContext, Charset encoding) {
                throw new UnsupportedOperationException("field and group ends not allowed in begin or end layout");

            case LayoutHelper.IS_OPTION_FIELD:
-                String field = BibDatabase.getText(text, databaseContext.getDatabase());
+                String field = Optional.ofNullable(databaseContext)
+                                       .map(BibDatabaseContext::getDatabase)


Maybe this could be null?

leaf-soba · 2024-09-30T00:17:22Z

Very small nitpick (because we are aiming for clean code)

One other comment. While thinking through. Did you check your new code with code coverage (by tests)?

Yes I did check it with old unit test code, but I think maybe we need more test. Sorry I'm a little busy yesterday, so I just wrote "need more unit test" in the commit message, I think I'll add more test today.

1. more unit test 2. rollback logic to follow old code which assumed that databaseContext is never null

koppor · 2024-10-01T06:43:34Z

src/test/java/org/jabref/logic/layout/LayoutEntryTest.java

+        // test IS_LAYOUT_TEXT
+        assertLayoutResult(1, "testString", parsedEntries, bibDatabaseContext);
+        // test IS_OPTION_FIELD with empty dataBase in BibDatabaseContext
+        assertLayoutResult(5, "testString", parsedEntries, bibDatabaseContext);
+        // test IS_ENCODING_NAME
+        assertLayoutResult(8, StandardCharsets.UTF_8.toString(), parsedEntries, bibDatabaseContext);
+        // test IS_FILENAME and IS_FILEPATH with empty path
+        assertLayoutResult(9, "", parsedEntries, bibDatabaseContext);
+        assertLayoutResult(10, "", parsedEntries, bibDatabaseContext);


Looks like @ParameterizedTest should be used. --> There should be only one assertion per test case. (More readings at https://devdocs.jabref.org/code-howtos/testing.html#general-hints-on-tests)

My account was flagged by automated systems while I didn't do anything violate the terms of service, finally I get my account back by sending ticket. Sorry for left this PR behind about 15 days.

Welcome back! - I started to miss you - good to hear that you are back! Looking forward for more code-quality pull requests. Especially many tests should be converted to @ParameterizedTests. - I can send you pointers if I come along of some of these.

change to @ParameterizedTest test

src/main/java/org/jabref/logic/layout/LayoutEntry.java

koppor · 2024-10-17T06:23:17Z

src/test/java/org/jabref/logic/layout/LayoutEntryTest.java

+        List<StringInt> parsedEntries = List.of(new StringInt("place_holder", 0),
+                new StringInt("testString", 0));
+        BibDatabaseContext bibDatabaseContext = new BibDatabaseContext(new BibDatabase(), new MetaData(), null);
+        assertUnsupportedOperation(type, parsedEntries, bibDatabaseContext);


Inline the helper method - that helper method is used only here. No need to have a separeate method for two liens of code.

koppor · 2024-10-17T06:23:24Z

src/test/java/org/jabref/logic/layout/LayoutEntryTest.java

+        List<StringInt> parsedEntries = List.of(new StringInt("place_holder", 0),
+                new StringInt("testString", 0));
+        BibDatabaseContext bibDatabaseContext = new BibDatabaseContext(new BibDatabase(), new MetaData(), null);
+        assertLayoutResult(type, expectedValue, parsedEntries, bibDatabaseContext);


Inline the helper method - that helper method is used only here. No need to have a separeate method for two liens of code.

koppor · 2024-10-17T06:24:06Z

src/test/java/org/jabref/logic/layout/LayoutEntryTest.java

+            "9, ''",
+            "10, ''"
+    })
+    void testLayoutResult(int type, String expectedValue) {


I wonder why OpenRewrite does not state anything (https://docs.openrewrite.org/recipes/java/testing/cleanup/removetestprefix)

--> test names should not start with test just remove test and strt with a lower case L.

I wonder why OpenRewrite does not state anything

Because it seems there is a conflict with main, the OpenRewrite didn't start to work in this situation.

koppor · 2024-10-17T06:24:14Z

src/test/java/org/jabref/logic/layout/LayoutEntryTest.java

+            "6",
+            "7"
+    })
+    void testUnsupportedOperationTypes(int type) {


Remove test prefix.

1. remove test prefix 2. remove unnecessary inline the helper method 3. fix a type 4. two minor refactor in both files.

remove import org.jspecify.annotations.Nullable;

* remove Deprecated getText() remove Deprecated getText() * Fix the code to pass unit test Maybe I should add more unit test based on my change? * Add more unit test to support my change 1. more unit test 2. rollback logic to follow old code which assumed that databaseContext is never null * change to @ParameterizedTest test change to @ParameterizedTest test * update follow requested change 1. remove test prefix 2. remove unnecessary inline the helper method 3. fix a type 4. two minor refactor in both files. * remove unused import remove import org.jspecify.annotations.Nullable; --------- Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>

remove Deprecated getText()

2ddfdac

remove Deprecated getText()

Fix the code to pass unit test

e94136f

Maybe I should add more unit test based on my change?

koppor requested changes Sep 29, 2024

View reviewed changes

Add more unit test to support my change

639f9bb

1. more unit test 2. rollback logic to follow old code which assumed that databaseContext is never null

leaf-soba requested a review from koppor September 30, 2024 14:37

koppor reviewed Oct 1, 2024

View reviewed changes

change to @ParameterizedTest test

daa55e4

change to @ParameterizedTest test

koppor requested changes Oct 17, 2024

View reviewed changes

update follow requested change

dec8709

1. remove test prefix 2. remove unnecessary inline the helper method 3. fix a type 4. two minor refactor in both files.

koppor previously approved these changes Oct 17, 2024

View reviewed changes

Merge branch 'main' into doing-issue-11674

34e718e

koppor dismissed their stale review via 34e718e October 17, 2024 07:23

koppor enabled auto-merge October 17, 2024 07:23

koppor previously approved these changes Oct 17, 2024

View reviewed changes

remove unused import

18cacf1

remove import org.jspecify.annotations.Nullable;

auto-merge was automatically disabled October 17, 2024 07:29
Head branch was pushed to by a user without write access

leaf-soba dismissed koppor’s stale review via 18cacf1 October 17, 2024 07:29

koppor enabled auto-merge October 17, 2024 07:33

koppor approved these changes Oct 17, 2024

View reviewed changes

koppor added this pull request to the merge queue Oct 17, 2024

Merged via the queue into JabRef:main with commit 62badb9 Oct 17, 2024
23 of 26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove Deprecated getText() #11841

remove Deprecated getText() #11841

leaf-soba commented Sep 27, 2024

koppor commented Sep 27, 2024

leaf-soba commented Sep 27, 2024

koppor commented Sep 29, 2024

koppor left a comment

koppor Sep 29, 2024

koppor Sep 29, 2024

koppor Sep 29, 2024

leaf-soba commented Sep 30, 2024 •

edited

Loading

koppor Oct 1, 2024

leaf-soba Oct 17, 2024

koppor Oct 17, 2024

koppor Oct 17, 2024

koppor Oct 17, 2024

koppor Oct 17, 2024

leaf-soba Oct 17, 2024

koppor Oct 17, 2024

remove Deprecated getText() #11841

remove Deprecated getText() #11841

Conversation

leaf-soba commented Sep 27, 2024

Mandatory checks

koppor commented Sep 27, 2024

leaf-soba commented Sep 27, 2024

koppor commented Sep 29, 2024

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leaf-soba commented Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leaf-soba commented Sep 30, 2024 •

edited

Loading