Skip to content

Added the feature of IcoRanking #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 3 commits into
base: main
Choose a base branch
from
Open

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.

Optional<String> finalBookTitle = bookTitle;
String rawInput = icoreField.orElseGet(() -> finalBookTitle.orElse("Unknown"));

Optional<String> acronym = ConferenceUtil.extractAcronym(rawInput); // Extracting the acronym from our input field
Copy link

Choose a reason for hiding this comment

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

Comment is trivial and can be derived from the code itself. The comment doesn't add any new information about why the acronym extraction is needed.

public ICoreRankingEditor(Field field) {
this.field = field;
this.textField = new TextField();
this.repo = new ICoreRankingRepository(); // Load once
Copy link

Choose a reason for hiding this comment

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

Comment 'Load once' is trivial and doesn't provide additional information. The single initialization in the constructor already implies this.

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.

Field should be final to prevent modification after initialization, following immutability best practices and thread-safety principles of modern Java.

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.

String note,
String dblp,
String primaryFor,
String averageRating
Copy link

Choose a reason for hiding this comment

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

Using String for numerical data prevents proper validation and comparison. Should use Double or BigDecimal to ensure proper numerical handling and prevent invalid data entry.

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. Maps should be initialized using Map.of() for empty maps instead of new HashMap<>(). This improves code readability and follows modern Java practices.

private Map<String, ConferenceRankingEntry> acronymMap = new HashMap<>();
private Map<String, ConferenceRankingEntry> nameMap = new HashMap<>();
private StringSimilarity similarity = new StringSimilarity();
private Logger LOGGER = LoggerFactory.getLogger(ICoreRankingRepository.class);
Copy link

Choose a reason for hiding this comment

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

Logger should be declared as private static final following Java naming conventions. Constants should be in uppercase, and loggers are typically static final members.

// 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 as it indicates a programming error. Additionally, try block covers too much code. It should be split into smaller, more focused blocks.

import java.util.regex.Pattern;

public class ConferenceUtil {
public static Optional<String> extractAcronym(String title) {
Copy link

Choose a reason for hiding this comment

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

Method parameter 'title' lacks null check, which could lead to NullPointerException when calling matcher. Method should validate input parameters to ensure robustness.

import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class ConferenceUtil {
Copy link

Choose a reason for hiding this comment

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

Utility class should have a private constructor to prevent instantiation, following effective Java principles for utility classes containing only static methods.

@@ -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 provides more concise and readable alternatives like List.of() for creating immutable lists.

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.

The added field introduces unnecessary blank line before it, violating the consistent formatting pattern used in the enum declaration.

@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

Copy link

trag-bot bot commented Jul 10, 2025

@trag-bot didn't find any issues in the code! ✅✨

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

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