Fixes and Extends BibliographyConsistencyCheck.check to Handle Custom Entry Types#14257
Conversation
Hey @Hrishi-Baskaran!Thank you for contributing to JabRef! Your help is truly appreciated ❤️. We have automatic checks in place, based on which you will soon get automated feedback if any of them are failing. We also use TragBot with custom rules that scans your changes and provides some preliminary comments, before a maintainer takes a look. TragBot is still learning, and may not always be accurate. In the "Files changed" tab, you can go through its comments and just click on "Resolve conversation" if you are sure that it is incorrect, or comment on the conversation if you are doubtful. Please re-check our contribution guide in case of any other doubts related to our contribution workflow. |
|
Please ensure the automatic check are green, otherwise no human will review it |
Thanks for telling me about this. I will update this on Friday. |
e5a97e0 to
e12bf76
Compare
Also modifies BibliographyConsistencyCheckTest to mock BibEntryTypes manager.
e12bf76 to
ba9446d
Compare
…entry-types-consistency-check-13794
| private final GuiPreferences preferences; | ||
| private final BibEntryTypesManager entryTypesManager; | ||
| private final UiTaskExecutor taskExecutor; | ||
| @Inject private BibEntryTypesManager bibEntryTypesManager; |
There was a problem hiding this comment.
We don't inject - we pass by constructor
There was a problem hiding this comment.
|
|
||
| BibliographyConsistencyCheck consistencyCheck = new BibliographyConsistencyCheck(); | ||
| BibliographyConsistencyCheck.Result result = consistencyCheck.check(databaseContext, (count, total) -> { | ||
| BibliographyConsistencyCheck.Result result = consistencyCheck.check(databaseContext, new BibEntryTypesManager(), (count, total) -> { |
There was a problem hiding this comment.
Refactor this as variable to follow the style of Jabef
There was a problem hiding this comment.
Also just curious: should I be constructing a new BibEntryTypesManager here like I've done? This basically prevents the user from using custom types when using the cli consistency check. How might we let users specify a entry types manager from the cli? Or maybe I'm missing something and this isn't what we want to do.
| // collects fields existing in any entry, scoped by entry type | ||
| Map<EntryType, Set<Field>> entryTypeToFieldsInAnyEntryMap = new HashMap<>(); | ||
| // collects fields existing in all entries, scoped by entry type | ||
| // collects fields existing in all entries, scoped by entry typed |
There was a problem hiding this comment.
|
|
||
| BibDatabaseContext bibContext = new BibDatabaseContext(database); | ||
| BibliographyConsistencyCheck.Result result = new BibliographyConsistencyCheck().check(bibContext, (count, total) -> { | ||
| BibliographyConsistencyCheck.Result result = new BibliographyConsistencyCheck().check(bibContext, new BibEntryTypesManager(), (count, total) -> { |
There was a problem hiding this comment.
Make this a variable in the test - and reuse it.
There was a problem hiding this comment.
I also made the mocked BibEntryTypesManager for this test file have some custom and unknown types (similar to what I did for BibliographyConsistencyCheckTest.java). But for this file, I don't use the custom and unknown types. Should I add some unit tests that use these, or remove these types for the mocked BibEntryTypesManager of the CSV test file?
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| // TODO: add some custom entry types for this manager and test with it |
There was a problem hiding this comment.
Yes, please :) - The issue description at #13794 should provide you with one.
There was a problem hiding this comment.
Also changes style and refactors some code to adhere to JabRef standards.
…entry-types-consistency-check-13794
|
This PR is being closed due to continued inactivity. |
|
Hello I'm very sorry about being inactive. I was going through a lot of other work I had to do and was planning on contributing again this weekend. Would it be possible to re-open this PR? |
…entry-types-consistency-check-13794
|
|
||
| BibliographyConsistencyCheck consistencyCheck = new BibliographyConsistencyCheck(); | ||
| BibliographyConsistencyCheck.Result result = consistencyCheck.check(databaseContext, (count, total) -> { | ||
| BibEntryTypesManager bibEntryTypesManager = new BibEntryTypesManager(); |
There was a problem hiding this comment.
No - pass this via constructor. See for instance org.jabref.gui.frame.SendAsStandardEmailAction#SendAsStandardEmailAction.
|
|
||
| if (clientHandler.getSettings().isConsistencyCheck()) { | ||
| consistencyDiagnosticsCache.put(uri, lspConsistencyCheck.check(parserResult)); | ||
| BibEntryTypesManager bibEntryTypesManager = new BibEntryTypesManager(); |
There was a problem hiding this comment.
bibEntryTypesManager should be passed via the constructor.
There was a problem hiding this comment.
This should be handled now
…entry-types-consistency-check-13794
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||
… of github.com:Hrishi-Baskaran/jabref into support-customized-entry-types-consistency-check-13794
…entry-types-consistency-check-13794
|
Your pull request conflicts with the target branch. Please merge with your code. For a step-by-step guide to resolve merge conflicts, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line. |
…entry-types-consistency-check-13794
…try-types-consistency-check-13794
koppor
left a comment
There was a problem hiding this comment.
Nice to see the handling of the OrFields
I fixed the conflcits. Good to go.
If there are any logical flaws, user will report - hopefully ^^
|
Thanks for fixing those remaining conflicts and merging the PR, you were such a great help during the entire process. I'll be ready to take on any user report issues about this, so please keep me in the loop. |
User description
Closes #13794
The current implementation of BibliographyConsistencyCheck.check is only aware of standard entry types of both BibTex and BibLaTex. The current implementation is not able to handle scenarios where the user defines a custom bibliography entry type. In this PR, BibliographyConsistencyCheck.check will be modified to handle consistency checks involving custom entry types in addition to new unit tests to cover the check for a variety of mocked custom entry types.
This PR also fixes a bug with the check function. The check function was also intended to check if required fields for a given type were set - returning entries that didn't have required fields set as inconsistencies. This was not mentioned in the javadoc but attempted anyway. Even then, the implementation was incorrect. In this PR, the implementation is updated to correctly determine if required fields are set. The Javadoc is also updated to explicitly mention this.
Steps to test
One can test this feature by running the tests in
BibliographyConsistencyCheckTest
In a live application, one can test this feature by going to Quality -> Check Consistency on a database. Ideally test on a database with custom entry types or with entry types that don't have required fields set to see the new update.
Mandatory checks
CHANGELOG.mdin a way that is understandable for the average user (if change is visible to the user)PR Type
Enhancement, Bug fix
Description
Consistency check now supports custom entry types and fields
Fixed required fields detection to properly handle OrFields constraints
Integrated BibEntryTypesManager throughout consistency check pipeline
Updated all callers to pass BibEntryTypesManager parameter
Enhanced test coverage with custom type scenarios
Diagram Walkthrough
File Walkthrough
1 files
Core consistency check refactored for custom types3 files
Comprehensive tests for custom entry typesUpdated CSV writer tests with required fieldsUpdated TXT writer tests with required fields9 files
Pass BibEntryTypesManager to consistency checkUse display name for entry typesInitialize LanguageServerController with entry types managerPass BibEntryTypesManager to consistency checkAccept BibEntryTypesManager in constructorThread LSP launcher with entry types managerStore and propagate BibEntryTypesManagerAccept BibEntryTypesManager parameterStore and use BibEntryTypesManager for diagnostics1 files
Document consistency check enhancements