Skip to content

Add ICORE Ranking support #13512

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add ICORE Ranking support #13512

wants to merge 2 commits into from

Conversation

asifebrahim
Copy link

@asifebrahim asifebrahim commented Jul 8, 2025

Closes #13476

This is the PR request for issue no #13476

Steps to test

In the General field at the very bottom there sthe icoranking lookup fiield there one can search using a acronym and get the details about the conference.
Screenshot from 2025-07-09 01-32-12
Screenshot from 2025-07-09 01-32-28

NOTE

-The Abbreviation tests were failing before i added any code so there were a total of 76 failed test case sand after my addition it is still 76 and all of them are NullPointerException and I didn't fix that

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.

@@ -11,6 +11,7 @@
requires java.sql.rowset;

// region JavaFX
requires javafx.swing;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The module requires both Swing and JavaFX dependencies, which violates the architectural decision to use only JavaFX for UI components. Swing dependencies should be removed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the dep here


@Test
void testEditorCreationDoesNotCrash() {
assertNotNull(FieldEditors.class);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to modern testing practices, should assert specific contents/behavior rather than just checking for non-null conditions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this test - it is always true

Comment on lines 4 to 12
public final String title;
public final String acronym;
public final String source;
public final String rank;
public final String note;
public final String dblp;
public final String primaryFor;
public final String comments;
public final String averageRating;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Class fields are declared as public, violating encapsulation principles. They should be private with getter methods to maintain proper encapsulation and control over data access.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convert the class to record

assertTrue(repo.getRankingFor("NON_EXISTENT").isEmpty());
}
@Test
public void test1(){
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test method name is not descriptive enough. It should clearly indicate what is being tested, such as 'testFuzzyMatchForInternationalConferenceOnSoftwareEngineering'.

assertEquals(Optional.empty(), ConferenceUtil.extractAcronym(title));
}
@Test
void test1(){
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test method name is not descriptive enough and doesn't indicate what is being tested. Should describe the test scenario like 'testExtractAcronymFromConferenceWithParentheses'.

assertEquals(Optional.of("ICSE"),ConferenceUtil.extractAcronym(title));
}
@Test
void test2(){
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test method name is not descriptive enough and doesn't indicate what is being tested. Should describe the test scenario like 'testExtractAcronymFromConferenceWithoutParentheses'.


@Test
void testEditorCreationDoesNotCrash() {
assertNotNull(FieldEditors.class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this test - it is always true

public void bindToEntry(BibEntry entry) {
this.currentEntry = entry;

// System.out.println("ENTRY booktitle = " + entry.getField(StandardField.BOOKTITLE).orElse("none"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional<String> acronym = ConferenceUtil.extractAcronym(rawInput); // Extracting the acronym from our input field
Optional<ConferenceRankingEntry> result = acronym.flatMap(repo::getFullEntry)
.or(() -> repo.getFullEntry(rawInput)); // Finding if any matching entry present in csv file
// If present then print the info
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reformat the comments -- starting at column 1 is not ok

this.setSpacing(10);
}

// Deprecated method it only shows rank by puttin the acronym in the text field
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what this code does.

I think, this should be removed -- it is part of some logic in all cases.

Primary FoR: %s
Comments: %s
Average Rating: %s
""", title, acronym, source, rank, note, dblp, primaryFor, comments, averageRating);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is OK to have an additional way of showing the contents...

However, in future, use IntelliJ's feature to create a toString method


try (BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8))) {
reader.lines().skip(1).forEach(line -> {
String[] parts = line.split(",(?=(?:[^\"]*\"[^\"]*\")*[^\"]*$)", -1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.

There are libraries for this. In JabRef, we use https://commons.apache.org/proper/commons-csv/

LOGGER.info("Fuzzy match fallback triggered for: " + key);
return nameToRank.entrySet().stream()
.filter(e -> similarity.editDistanceIgnoreCase(e.getKey(), key) < 0.01)
.peek(e -> LOGGER.info("Fuzzy match candidate: " + e.getKey()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong logger format - use {} as place holder. See other code in JabRef.

(also applies for all other logger statements)

Comment on lines 4 to 12
public final String title;
public final String acronym;
public final String source;
public final String rank;
public final String note;
public final String dblp;
public final String primaryFor;
public final String comments;
public final String averageRating;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convert the class to record

@koppor
Copy link
Member

koppor commented Jul 9, 2025

-The Abbreviation tests were failing before i

Maybe, you did not check out JabRef correctly. Execute git submodule update on the command line.

@koppor
Copy link
Member

koppor commented Jul 9, 2025

Regarding the output information

image

  • 1: Should be link -- See other places in JabRef how to open a browser
  • 2: Remove - this information is not useful

@koppor
Copy link
Member

koppor commented Jul 9, 2025

@asifebrahim You screenshort shows Icoranking as field name. It is ICORE, which should be rendered as Icoreranking in JabRef. You missed two letters. Also in your title in this PR, you missed two letters. Please engineer properly 😅.

@koppor
Copy link
Member

koppor commented Jul 9, 2025

Even though the larger button is more accessible - the current JabRef style is to use icons. Please use the same button style as in the DOI

image

@koppor
Copy link
Member

koppor commented Jul 9, 2025

You also need to do a preference migration - icoreranking needs to be added to general

image

  • Add a new method to org.jabref.migrations.PreferencesMigrations
  • Method is similar to org.jabref.migrations.PreferencesMigrations#removeCommentsFromCustomEditorTabs
  • Method has to check if the current content is General:doi;crossref;keywords;eprint;url;file;groups;owner;timestamp;printed;priority;qualityassured;ranking;readstatus;relevance. If yes, change it to doi;icoreranking;...

Also add the field directly after DOI to org.jabref.model.entry.field.FieldFactory#getDefaultGeneralFields.

@koppor
Copy link
Member

koppor commented Jul 10, 2025

@asifebrahim Before requesting a review, please fix the tests. Although it seems, I have endless time, in reality, I don't. I like automation and that machines can support us all in making good code. Thus, please, look at the failing tests.

image


public class ConferenceUtil {
public static Optional<String> extractAcronym(String title) {
Matcher matcher = Pattern.compile("\\((.*?)\\)").matcher(title);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pattern needs to be a private static final variable to be really speed saving

@jabref-machine
Copy link
Collaborator

You committed your code on the main brach of your fork. This is a bad practice. The right way is to branch out from main, work on your patch/feature in that new branch, and then get that branch merged via the pull request (see GitHub flow).

For this pull request, this is OK. For subsequent pull requests, please start with a different branch with a proper branch name. See CONTRIBUTING.md for more details.

@jabref-machine
Copy link
Collaborator

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

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please adress my review comments - and also the commetns by trag-bot.

Expecially:

There are libraries for this. In JabRef, we use commons.apache.org/proper/commons-csv

@jabref-machine
Copy link
Collaborator

Hey, we noticed that you force-pushed your changes. Force pushing is a bad practice when working together on a project (mainly because it is not supported well by GitHub itself). Commits are lost and comments on commits lose their context, thus making it harder to review changes. At the end, all commits will be squashed anyway before being merged into the main branch.

In future, please avoid that. For now, you can continue working.


this.textField.setPromptText("Enter or lookup ICORE rank");

// Button lookupButton = new Button("Lookup Rank");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented code should be removed as it serves no purpose and clutters the codebase. Version control should be used to track code history instead.

Comment on lines +73 to +74
alert.setTitle("ICORE Ranking Info");
alert.setHeaderText("Found Conference Details");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UI text uses title case instead of sentence case as required by project guidelines for consistency in the user interface.

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class PreferencesMigrations {

private static final Logger LOGGER = LoggerFactory.getLogger(PreferencesMigrations.class);
private JabRefGuiPreferences preferences;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instance field introduces unnecessary state to what should be a utility class. This violates the principle of keeping utility classes stateless and thread-safe.

import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.*;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using wildcard imports instead of specific imports reduces code readability and can lead to naming conflicts. Each import should be explicitly declared.

@@ -0,0 +1,26 @@
package org.jabref.logic.icore;

public record ConferenceRankingEntry(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The record lacks input validation for required fields. Some fields like title or acronym should not be null. Consider adding compact constructor with validation.

Comment on lines +19 to +22
final Map<String, String> acronymToRank = new HashMap<>();
private Map<String, String> nameToRank = new HashMap<>();
private Map<String, ConferenceRankingEntry> acronymMap = new HashMap<>();
private Map<String, ConferenceRankingEntry> nameMap = new HashMap<>();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use modern Java data structures. HashMap initialization can be replaced with more concise Map.of() for empty maps, following modern Java practices.

// System.out.println("Loaded entries:");
// acronymToRank.forEach((key, val) -> System.out.println("Acronym: " + key + " -> " + val));
// nameToRank.forEach((key, val) -> System.out.println("Name: " + key + " -> " + val));
} catch (NullPointerException | IOException e) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NullPointerException should not be caught explicitly as it indicates a programming error. Additionally, only logging debug for a critical data loading failure is insufficient.

@@ -231,7 +231,7 @@ private static Set<Field> getAllFields() {
* separate preferences object
*/
public static List<Field> getDefaultGeneralFields() {
List<Field> defaultGeneralFields = new ArrayList<>(Arrays.asList(StandardField.DOI, StandardField.CROSSREF, StandardField.KEYWORDS, StandardField.EPRINT, StandardField.URL, StandardField.FILE, StandardField.GROUPS, StandardField.OWNER, StandardField.TIMESTAMP));
List<Field> defaultGeneralFields = new ArrayList<>(Arrays.asList(StandardField.DOI, StandardField.ICORERANKING, StandardField.CROSSREF, StandardField.KEYWORDS, StandardField.EPRINT, StandardField.URL, StandardField.FILE, StandardField.GROUPS, StandardField.OWNER, StandardField.TIMESTAMP));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using ArrayList with Arrays.asList is an outdated pattern. Modern Java practices suggest using List.of() for creating immutable lists, which is more concise and efficient.

@jabref-machine
Copy link
Collaborator

JUnit tests of jablib are failing. You can see which checks are failing by locating the box "Some checks were not successful" on the pull request page. To see the test output, locate "Tests / Unit tests (pull_request)" and click on it.

You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.

Comment on lines +142 to +143
MODIFICATIONDATE("modificationdate", FieldProperty.DATE),
ICORERANKING("icoreranking");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra blank line added between fields without adding new statements, which violates code formatting consistency within the enum declaration.

@koppor koppor changed the title Added the feature of IcoRanking Add ICORE Ranking support Jul 11, 2025
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more we iterate the more non-working AI-generated code seems to come in.

Please, either take this as programming exercise or free the issue for someone else making the impression wanting to learn how to craft high-quality java code. - At least, we would like to have the impression that this is taken serious.

ConferenceRankingEntry entry = result.get();

// Show in new dialog
javafx.scene.control.Alert alert = new javafx.scene.control.Alert(javafx.scene.control.Alert.AlertType.INFORMATION);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, this is for debugging and can be removed?

Optional<String> icoreField = currentEntry.getField(StandardField.ICORERANKING);
Optional<String> bookTitle = currentEntry.getFieldOrAlias(StandardField.BOOKTITLE);
if (bookTitle.isEmpty()) {
bookTitle = currentEntry.getField(StandardField.JOURNAL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also getFieldOrAlias

bookTitle = currentEntry.getField(StandardField.JOURNAL);
}

Optional<String> finalBookTitle = bookTitle;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this?

@@ -562,7 +557,21 @@ static void moveApiKeysToKeyring(JabRefCliPreferences preferences) {
* The tab "Comments" is hard coded using {@link CommentsTab} since v5.10 (and thus hard-wired in {@link org.jabref.gui.entryeditor.EntryEditor#createTabs()}.
* Thus, the configuration ih the preferences is obsolete
*/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some AI tool always adds an empty line above the method - which AI tool is this?

This is wrong!

@Test
public void printAllAcronyms() {
ICoreRankingRepository repo = new ICoreRankingRepository();
repo.acronymToRank.forEach((key, value) -> System.out.println(key + " => " + value));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is NOT a test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for automatic ICORE conference ranking lookup
4 participants