feat:Allowed multiple delimiter in keyword separator field. #13706

tushar-2320 · 2025-08-17T04:54:00Z

Closes #12974

Mandatory checks

I own the copyright of the code submitted and I license it under the MIT license
Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (if change is visible to the user)
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.

koppor · 2025-08-17T06:56:28Z

The approach seems to be wrong.

Do test-driven development. Add tests for each fetcher. At least for the one mentioned in the issue.

You can do heuristics. On the fetcher side. Not on the keywords class.

tushar-2320 · 2025-08-17T12:40:01Z

can you give me some example for the tests and where is this fetcher located.

tushar-2320 · 2025-08-18T14:21:26Z

can you give me some example for the tests and where is this fetcher located.

@koppor @subhramit

InAnYan · 2025-08-18T20:18:41Z

It's in the jablib project.
In src/test. src/test/java classes generally follow the same "paths" as in src/main/java. So, if you want to find tests for src/main/java/a/b/c.java, then it's typically at src/test/java/a/b/cTest.java

tushar-2320 · 2025-08-25T04:58:24Z

@calixtus @koppor I think I have refactored too much. is this the way?

jabref-machine · 2025-08-25T04:58:47Z

Your pull request conflicts with the target branch.

Please merge upstream/main with your code. For a step-by-step guide to resolve merge conflicts, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line.

trag-bot · 2025-08-25T05:03:14Z

jablib/src/main/java/org/jabref/logic/citationkeypattern/BracketedPattern.java

@@ -481,7 +481,15 @@ public static String getFieldValue(BibEntry entry, String pattern, Character key
            } else if (pattern.matches("keyword\\d+")) {
                // according to LabelPattern.php, it returns keyword number n
                int num = Integer.parseInt(pattern.substring(7));
-                KeywordList separatedKeywords = entry.getResolvedKeywords(keywordDelimiter, database);
+                // KeywordList separatedKeywords = entry.getResolvedKeywords(keywordDelimiter, database);


Commented code should be removed as it serves no purpose in understanding the current implementation and clutters the codebase. Git history should be used to track changes.

trag-bot · 2025-08-25T05:03:17Z

jablib/src/main/java/org/jabref/logic/citationkeypattern/BracketedPattern.java

@@ -481,7 +481,15 @@ public static String getFieldValue(BibEntry entry, String pattern, Character key
            } else if (pattern.matches("keyword\\d+")) {
                // according to LabelPattern.php, it returns keyword number n
                int num = Integer.parseInt(pattern.substring(7));
-                KeywordList separatedKeywords = entry.getResolvedKeywords(keywordDelimiter, database);
+                // KeywordList separatedKeywords = entry.getResolvedKeywords(keywordDelimiter, database);


Another instance of commented code that should be removed. Version control systems like Git are designed to maintain code history, making commented-out code unnecessary.

calixtus · 2025-08-25T06:00:09Z

Thanks for your interest in jabref.
I believe just replacing character with string is the wrong approach. This may work on a from a simple programming perspective. But you need arcane knowledge to deal with that string. One must know that every character in the string is a delimiter.

If you want to push around a list of multiple delimiters, use a list. Keep the level of abstraction. Maybe one wants du use "./." As a delimiter?

Also please add tests / use test driven development as @koppor suggests.
Create test cases, create tests, see them fail, fix the codebase.

tushar-2320 · 2025-08-25T09:57:28Z

Thanks for your interest in jabref. I believe just replacing character with string is the wrong approach. This may work on a from a simple programming perspective. But you need arcane knowledge to deal with that string. One must know that every character in the string is a delimiter.

If you want to push around a list of multiple delimiters, use a list. Keep the level of abstraction. Maybe one wants du use "./." As a delimiter?

Also please add tests / use test driven development as @koppor suggests. Create test cases, create tests, see them fail, fix the codebase.

actually I am facing difficulty in that .
I dont know in which test file I need to write the test case can you give me a particular file name @calixtus ?

calixtus · 2025-08-25T21:49:27Z

KeywordListTest.java

This reverts commit 88fe7f7.

koppor · 2025-08-31T20:54:17Z

KeywordListTest.java

First, I was not sure if this OK - however, the KeywordList can have different parsing modes. Maybe with a heuristics to find out the keyword seperator - then this list can be serialized again and put into the BibEntry returned by the fetcher.

jabref-machine · 2025-09-02T04:57:42Z

Note that your PR will not be reviewed/accepted until you have gone through the mandatory checks in the description and marked each of them them exactly in the format of [x] (done), [ ] (not done yet) or [/] (not applicable).

trag-bot · 2025-09-02T04:59:59Z

jablib/src/main/java/org/jabref/model/entry/KeywordList.java

@@ -73,6 +94,10 @@ public static KeywordList parse(String keywordString, Character delimiter) {
        return parse(keywordString, delimiter, Keyword.DEFAULT_HIERARCHICAL_DELIMITER);
    }

+    public static KeywordList parseMultipleDelimeter(String keywordString, String delimiter) {


Method name contains a spelling error ('Delimeter' instead of 'Delimiter'), which violates consistent and correct naming conventions in the codebase.

trag-bot · 2025-09-02T05:00:00Z

jablib/src/main/java/org/jabref/model/entry/KeywordList.java

+        for (char d:delimiter.toCharArray()) {
+            keywordString = keywordString.replace(d, ',');
+        }


String mutation in loop is inefficient. Using StringBuilder or StringJoiner would be more performant for string manipulations.

trag-bot · 2025-09-02T05:00:40Z

jablib/src/test/java/org/jabref/model/entry/KeywordListTest.java

+    @Test
+    void parseMultipleDelimiter() {
+        assertEquals(new KeywordList("keywordOne", "keywordTwo", "keywordThree"),
+                KeywordList.parseMultipleDelimeter("keywordOne; keywordTwo: keywordThree", ";:"));


Method name contains a typo ('Delimeter' instead of 'Delimiter'), which violates the requirement for correctly spelled variable and method names.

initial commit

27f97b5

tushar-2320 marked this pull request as draft August 17, 2025 04:54

tushar-2320 changed the title ~~initial commit~~ feat:Allowed multiple delimiter in keyword separator field. Aug 17, 2025

calixtus added the component: keywords label Aug 20, 2025

refactor:converted arguments to string for multiple delimeter.

88fe7f7

trag-bot bot reviewed Aug 25, 2025

View reviewed changes

Revert "refactor:converted arguments to string for multiple delimeter."

729e2dc

This reverts commit 88fe7f7.

fix:created new method for parsing with multiple delimeter.

37bd7e2

trag-bot bot reviewed Sep 2, 2025

View reviewed changes

Uh oh!

feat:Allowed multiple delimiter in keyword separator field. #13706

Are you sure you want to change the base?

feat:Allowed multiple delimiter in keyword separator field. #13706

Conversation

tushar-2320 commented Aug 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Mandatory checks

Uh oh!

koppor commented Aug 17, 2025

Uh oh!

tushar-2320 commented Aug 17, 2025

Uh oh!

tushar-2320 commented Aug 18, 2025

Uh oh!

InAnYan commented Aug 18, 2025

Uh oh!

tushar-2320 commented Aug 25, 2025

Uh oh!

jabref-machine commented Aug 25, 2025

Uh oh!

trag-bot bot Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

trag-bot bot Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

calixtus commented Aug 25, 2025

Uh oh!

tushar-2320 commented Aug 25, 2025

Uh oh!

calixtus commented Aug 25, 2025

Uh oh!

koppor commented Aug 31, 2025

Uh oh!

jabref-machine commented Sep 2, 2025

Uh oh!

trag-bot bot Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

trag-bot bot Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

trag-bot bot Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tushar-2320 commented Aug 17, 2025 •

edited

Loading