-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add ICORE Ranking support #13512
Conversation
| 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 |
koppor
left a comment
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.
| 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.
| */ | ||
| 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.
koppor
left a comment
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?
| * 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
|
@asifebrahim May I ask if you will follow-up on this or if we should free this PR for other contributors? |
|
sorry for the delay i was facing some personal issues so couldn't work on that ill be working from tomorroww ill fix all the bugs that as spotted |
Did you find any time for this? Its also OK to work on in some weeks, but just let us know! This should be finished this year, preferably before September. |
|
ok ill finish this befor august no problm |
|
if you here no response or update from me till aug 4 remove me |
- Add ICORERankingEditor and ICORERankingEditorViewModel classes (inspired by other editors and their view models) to the GUI package. - Add ICORERankingEditor.fxml for the editor's layout. - Add field creation logic to FieldEditors#getForField. - Update jablib/module-info to export the icore logic and model packages for use on the GUI side. - Add ICORE rank field to FieldFactory#getDefaultGeneralFields and update preferences migrations as per JabRef#13512 (comment). - Fix typos in ConferenceAcronymExtractorTest. - Fix order of modifiers in ConferenceEntry. - Update CHANGELOG. Part of JabRef#13476
…13699) * Port DuplicateCheck.similarity to StringSimilarity and add tests * Add ICORE2023 Rank Data To Resources Data sourced from ICORE website here: https://portal.core.edu.au/conf-ranks/ to enable ICORE rank lookups. As discussed here: #13699 (comment), only the latest data from ICORE is to be used. At this time, it is the ICORE2023 ranking data. Part of #13476 * Add Core Classes For ICORE Rank Lookup - Append a header row to resources/icore/ICORE2023.csv - Add ConferenceEntry record to represent ICORE conference data - Add ConferenceRepository to load conference data and allow conference lookups using an acronym or a bookTitle with fuzzy match as a fallback - Add utility class to extract an acronym from a bookTitle - Add tests Part of #13476 * Integrate ICORE Rank Lookup Feature Into The GUI - Add ICORERankingEditor and ICORERankingEditorViewModel classes (inspired by other editors and their view models) to the GUI package. - Add ICORERankingEditor.fxml for the editor's layout. - Add field creation logic to FieldEditors#getForField. - Update jablib/module-info to export the icore logic and model packages for use on the GUI side. - Add ICORE rank field to FieldFactory#getDefaultGeneralFields and update preferences migrations as per #13512 (comment). - Fix typos in ConferenceAcronymExtractorTest. - Fix order of modifiers in ConferenceEntry. - Update CHANGELOG. Part of #13476 * Add Missing Localization Keys and Fix Broken Test - Out of the 4 missing keys in a failing test, two new keys were added to JabRef_en.properties while two were edited adapted to already present ones. - The performExportForSingleEntry test in OpenOfficeDocumentCreatorTest was failing because of the missing "Icoreranking" field. Further, since the ordering of JabRef-specific fields was changed in a previous commit to conform to alphabetical order, the hardcoded values in OldOpenOfficeCalcExportFormatContentSingleEntry.xml weren't matching with the exporter's output. This commit reorders the fields in the expected order and adds the "Icoreranking" field at its right place which fixes the broken test. Part of #13476 * Use List.of() and fix grammar * Reorder fields * Fix unsupported operation exception * Rename field * Port DuplicateCheck.similarity to StringSimilarity and add tests * Add ICORE2023 Rank Data To Resources Data sourced from ICORE website here: https://portal.core.edu.au/conf-ranks/ to enable ICORE rank lookups. As discussed here: #13699 (comment), only the latest data from ICORE is to be used. At this time, it is the ICORE2023 ranking data. Part of #13476 * Add Core Classes For ICORE Rank Lookup - Append a header row to resources/icore/ICORE2023.csv - Add ConferenceEntry record to represent ICORE conference data - Add ConferenceRepository to load conference data and allow conference lookups using an acronym or a bookTitle with fuzzy match as a fallback - Add utility class to extract an acronym from a bookTitle - Add tests Part of #13476 * Fix Merge Conflict From Upstream Fetch Part of #13476 * Add Missing Localization Keys and Fix Broken Test - Out of the 4 missing keys in a failing test, two new keys were added to JabRef_en.properties while two were edited adapted to already present ones. - The performExportForSingleEntry test in OpenOfficeDocumentCreatorTest was failing because of the missing "Icoreranking" field. Further, since the ordering of JabRef-specific fields was changed in a previous commit to conform to alphabetical order, the hardcoded values in OldOpenOfficeCalcExportFormatContentSingleEntry.xml weren't matching with the exporter's output. This commit reorders the fields in the expected order and adds the "Icoreranking" field at its right place which fixes the broken test. Part of #13476 * Add Minor Fixes, Documentation, and Refactor - Update ICORERankingEditorViewModel to display a notification when a conference ranking isn't found instead of displaying the "not found" text in the field. - Refactored PreferencesMigrations.addICORERankingFieldToGeneralTab to better align with JabRefCliPreferences.setLanguageDependentDefaultValues. - Minor refactor and remove redundant comment and whitespace in ConferenceRepository. - Update tests to Parameterized tests in ConferenceRepositoryTest and ConferenceAcronymExtractorTest. - Removed unused variables in ICORERankingEditor and ICORERankingEditorViewModel. - Update OldOpenOfficeCalcExportFormatContentSingleEntry.xml to align with the field ordering in StandardField. - Add documentation to PreferencesMigrations.addICORERankingFieldToGeneralTab and ConferenceAcronymExtractor.extract methods. Part of #13476 * Revert "Hotfix: calling of publish.yml" This reverts commit 75d2b47 which I accidentally pulled on my feature branch. * Remove duplicate line in CHANGELOG Part of #13476 * Improve Fuzzy Search Algorithm And Fix Undo Bug - Added utility functions to ConferenceUtils for acronym candidate generation and string normalization - Updated fuzzy search algorithm in ConferenceRepository to include Longest Common Substring similarity combined with Levenshtein similarity - Fixed bug in ICORERankingEditor which was causing Undo to not work - Updated ICORERankingEditorViewModel to use the leaner ConferenceRepository API - Added LCSSimilarity method to StringSimilarity to compute Longest Common Substring similarity rating - Added Javadoc to various non-trivial methods - Add tests to cover the updated functionality Part of #13476 * Discard changes to .github/workflows/tests-code.yml * Fix Broken Tests and Malformed JavaDoc Part of #13476 * Update Test In ConferenceUtilsTest Part of #13476 * Add Minor Fixes To ConferenceUtils Part of #13476 --------- Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>




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