-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
base: main
Are you sure you want to change the base?
Add ICORE Ranking support #13512
Conversation
@@ -11,6 +11,7 @@ | |||
requires java.sql.rowset; | |||
|
|||
// region JavaFX | |||
requires javafx.swing; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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(){ |
There was a problem hiding this comment.
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(){ |
There was a problem hiding this comment.
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(){ |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use LOGGER.debug - see https://devdocs.jabref.org/code-howtos/logging.html
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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())) |
There was a problem hiding this comment.
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)
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; |
There was a problem hiding this comment.
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
Maybe, you did not check out JabRef correctly. Execute |
@asifebrahim You screenshort shows |
You also need to do a preference migration -
Also add the field directly after |
@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. |
|
||
public class ConferenceUtil { | ||
public static Optional<String> extractAcronym(String title) { | ||
Matcher matcher = Pattern.compile("\\((.*?)\\)").matcher(title); |
There was a problem hiding this comment.
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
You committed your code on the 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. |
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 |
There was a problem hiding this 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
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 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"); |
There was a problem hiding this comment.
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.
alert.setTitle("ICORE Ranking Info"); | ||
alert.setHeaderText("Found Conference Details"); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.*; |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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.
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<>(); |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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.
JUnit tests of 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. |
MODIFICATIONDATE("modificationdate", FieldProperty.DATE), | ||
ICORERANKING("icoreranking"); |
There was a problem hiding this comment.
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.
There was a problem hiding this 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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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 | |||
*/ | |||
|
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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
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.


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
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)