Add quality checker for journal abbrevs#13190
Conversation
|
This PR fixes JabRef/abbrv.jabref.org#149. Update: I think, it is OK to have in here - and therefore in JabKit. But then, it should be wired into JabKit! Old text: It should be a pull request to https://github.com/JabRef/abbrv.jabref.org/. I like that it is implemented in Java. It can be implemented using JBang as wrapper. You can also use gradle or bld as build tool. |
@koppor Hi! I was thinking of adding a Runtime validation for when abbreviations are added dynamically and a CI bulk validation (via JabKit) to check all existing abbreviation files. Do you think it would be okay to do it that way? |
Yes. Also enable checking in the dialog (if possible) -> the lists can be modified eternally. |
| if (viewModel.validateAbbreviationsProperty().get()) { | ||
| AbbreviationViewModel item = event.getRowValue(); | ||
| String newValue = event.getNewValue(); | ||
| List<ValidationResult> results = viewModel.validateAbbreviation(item.getName(), newValue, item.getAbbreviation()); |
There was a problem hiding this comment.
The code duplicates the validation logic across multiple columns. This violates the DRY principle and could be refactored to a single method to improve maintainability.
| return uiCommands; | ||
| } | ||
|
|
||
| private void validateJournalAbbreviations() { |
There was a problem hiding this comment.
The method validateJournalAbbreviations does not follow the Single-responsibility principle as it both loads the repository and prints validation results. These should be separate responsibilities.
|
|
||
| private void printValidationResults(List<JournalAbbreviationValidator.ValidationResult> issues) { | ||
| if (issues.isEmpty()) { | ||
| System.out.println(Localization.lang("No validation issues found in journal abbreviations")); |
There was a problem hiding this comment.
The string 'No validation issues found in journal abbreviations' should be localized using Localization.lang to ensure multilingual support.
| try { | ||
| if (writeXMP) { | ||
| if (xmpPdfExporter.exportToAllFilesOfEntry(databaseContext, filePreferences, entry, List.of(entry), abbreviationRepository)) { | ||
| System.out.printf("Successfully written XMP metadata on at least one linked file of %s%n", citeKey); |
There was a problem hiding this comment.
The string should be localized using Localization.lang to ensure multilingual support.
| if (xmpPdfExporter.exportToAllFilesOfEntry(databaseContext, filePreferences, entry, List.of(entry), abbreviationRepository)) { | ||
| System.out.printf("Successfully written XMP metadata on at least one linked file of %s%n", citeKey); | ||
| } else { | ||
| System.err.printf("Cannot write XMP metadata on any linked files of %s. Make sure there is at least one linked file and the path is correct.%n", citeKey); |
There was a problem hiding this comment.
The string should be localized using Localization.lang to ensure multilingual support.
| } | ||
| if (embedBibfile) { | ||
| if (embeddedBibFilePdfExporter.exportToAllFilesOfEntry(databaseContext, filePreferences, entry, List.of(entry), abbreviationRepository)) { | ||
| System.out.printf("Successfully embedded metadata on at least one linked file of %s%n", citeKey); |
There was a problem hiding this comment.
The string should be localized using Localization.lang to ensure multilingual support.
| if (embeddedBibFilePdfExporter.exportToAllFilesOfEntry(databaseContext, filePreferences, entry, List.of(entry), abbreviationRepository)) { | ||
| System.out.printf("Successfully embedded metadata on at least one linked file of %s%n", citeKey); | ||
| } else { | ||
| System.out.printf("Cannot embed metadata on any linked files of %s. Make sure there is at least one linked file and the path is correct.%n", citeKey); |
There was a problem hiding this comment.
The string should be localized using Localization.lang to ensure multilingual support.
| for (String citeKey : citeKeys) { | ||
| List<BibEntry> bibEntryList = databaseContext.getDatabase().getEntriesByCitationKey(citeKey); | ||
| if (bibEntryList.isEmpty()) { | ||
| System.err.printf("Skipped - Cannot find %s in library.%n", citeKey); |
There was a problem hiding this comment.
The string should be localized using Localization.lang to ensure multilingual support.
| try { | ||
| if (writeXMP) { | ||
| if (xmpPdfExporter.exportToFileByPath(databaseContext, filePreferences, filePath, abbreviationRepository)) { | ||
| System.out.printf("Successfully written XMP metadata of at least one entry to %s%n", fileName); |
There was a problem hiding this comment.
The string should be localized using Localization.lang to ensure multilingual support.
| if (xmpPdfExporter.exportToFileByPath(databaseContext, filePreferences, filePath, abbreviationRepository)) { | ||
| System.out.printf("Successfully written XMP metadata of at least one entry to %s%n", fileName); | ||
| } else { | ||
| System.out.printf("File %s is not linked to any entry in database.%n", fileName); |
There was a problem hiding this comment.
The string should be localized using Localization.lang to ensure multilingual support.
| } | ||
| if (embeddBibfile) { | ||
| if (embeddedBibFilePdfExporter.exportToFileByPath(databaseContext, filePreferences, filePath, abbreviationRepository)) { | ||
| System.out.printf("Successfully embedded XMP metadata of at least one entry to %s%n", fileName); |
There was a problem hiding this comment.
The string should be localized using Localization.lang to ensure multilingual support.
| if (xmpPdfExporter.exportToFileByPath(databaseContext, filePreferences, filePath, abbreviationRepository)) { | ||
| System.out.printf("Successfully written XMP metadata of at least one entry to %s%n", fileName); | ||
| } else { | ||
| System.out.printf("File %s is not linked to any entry in database.%n", fileName); |
There was a problem hiding this comment.
The string should be localized using Localization.lang to ensure multilingual support.
| StringBuilder sb = new StringBuilder(); | ||
|
|
||
| int maxLength = table.stream() | ||
| .mapToInt(pair -> Objects.requireNonNullElse(pair.getKey(), "").length()) |
There was a problem hiding this comment.
The use of Objects.requireNonNullElse is discouraged. Instead, use JSpecify @nonnull annotation to ensure non-null values.
| String abbreviation = "Problemy Mat."; | ||
|
|
||
| var result = validator.checkWrongEscape(fullName, abbreviation, 1).orElseThrow(); | ||
| assertEquals(false, result.isValid()); |
There was a problem hiding this comment.
The test uses assertEquals to check a boolean condition. It should directly assert the boolean value using assertFalse for better readability and adherence to best practices.
| String abbreviation = "J. with \\r return and \\b backspace"; | ||
|
|
||
| var result = validator.checkWrongEscape(fullName, abbreviation, 1).orElseThrow(); | ||
| assertEquals(true, result.isValid()); |
There was a problem hiding this comment.
The test uses assertEquals to check a boolean condition. It should directly assert the boolean value using assertTrue for better readability and adherence to best practices.
|
Your code currently does not meet JabRef's code guidelines. We use Checkstyle to identify issues. 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 / Checkstyle (pull_request)" and click on it. In case of issues with the import order, double check that you activated Auto Import. You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports. Please carefully follow the setup guide for the codestyle. Afterwards, please run checkstyle locally and fix the issues, commit, and push. |
|
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. |
|
@trag-bot didn't find any issues in the code! ✅✨ |
|
@aaspst I see only red crosses here. Don't know why. Do you have time and motivatoin to continue working on this? |
|
The PR contains Maybe, start from scratch. One PR: Basic testing infrastructure and then subsequent PRs for each test. |
Closes JabRef/abbrv.jabref.org#149
Describe the changes you have made here: what, where, why, ...
Changes made
JournalAbbreviationValidatorclass with comprehensive validation checks for:"Zeszyty Naukowe Wy\")###Testing
JournalAbbreviationValidatorTestcovering:Acta Phys. Polon.)Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if change is visible to the user)