Skip to content

Conversation

@dcarpentiero
Copy link
Contributor

@dcarpentiero dcarpentiero commented Jul 24, 2025

Closes #13467

This draft PR refactors BibliographyConsistencyCheck to take a full BibDatabaseContext instead of just a list of entries.
The goal is to prepare the logic for BibTeX/BibLaTeX-aware consistency checking, as discussed in the issue.
So far, logic separation for mode (BibDatabaseMode) and extraction of EntryType has been implemented.

Steps to test

This is a work-in-progress. No final test procedure yet.
Initial behavior should be unchanged (functional refactoring).
Can be tested with a .bib file containing @online entries with and without date.

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.

@dcarpentiero
Copy link
Contributor Author

@subhramit hi that's me. I deleted the old branch with the old rebase problem and i've started clean.

@dcarpentiero
Copy link
Contributor Author

Hello all, I'd like to get some help regarding a failing test in jablib which seems unrelated to my modifications. Since this seems to fall outside the scope of the issue I'm addressing, how would you like me to proceed?
Thanks for the replies!

@dcarpentiero dcarpentiero marked this pull request as ready for review July 24, 2025 13:42
@subhramit subhramit changed the title Mode aware consistency check #13467 clean Mode aware consistency check Jul 24, 2025
@subhramit
Copy link
Member

@subhramit hi that's me. I deleted the old branch with the old rebase problem and i've started clean.

It's fine this time - but for future, we generally don't recommend closing and opening - one can update the existing branch so that the PR is also updated.

@subhramit
Copy link
Member

subhramit commented Jul 24, 2025

Hello all, I'd like to get some help regarding a failing test in jablib which seems unrelated to my modifications. Since this seems to fall outside the scope of the issue I'm addressing, how would you like me to proceed?
Thanks for the replies!

Open the failing check by clicking on it, compare the expected and actual values in the log.
Then trace back how your changes caused this.

As far as I can see:

org.jabref.logic.quality.consistency.BibliographyConsistencyCheckResultCsvWriterTest

 Test checkComplexLibrary(Path) FAILED

  org.opentest4j.AssertionFailedError: expected: <entry type,citation key,Location,Pages,Publisher
  Article,first,-,o,-
  Article,second,-,-,?
  InProceedings,fourth,-,-,o
  InProceedings,third,?,o,-
  > but was: <entry type,citation key,Pages,Publisher
  Article,first,o,-
  Article,second,-,?
  >

Some things seem to be missing from the actual.
Similarly:

org.jabref.logic.quality.consistency.BibliographyConsistencyCheckTest

  Test unsetFieldsReported() FAILED

  org.opentest4j.AssertionFailedError: expected: <[@online{withDate,
    date = {date},
    urldate = {urldate},
    _jabref_shared = {sharedId: -1, version: 1}
  }, @online{withoutDate,
    urldate = {urldate},
    _jabref_shared = {sharedId: -1, version: 1}
  }]> but was: <[@online{withDate,
    date = {date},
    urldate = {urldate},
    _jabref_shared = {sharedId: -1, version: 1}
  }]>

@subhramit
Copy link
Member

Mandatory checks

  • I own the copyright ...
  • [/] Change in CHANGELOG.md ...
  • Tests created ...
  • [/] Manually tested ...
  • [/] Screenshots added ...
  • [/] Checked developer's documentation ...
  • [/] Checked documentation ...

Why are the mandatory checks trimmed with "..." by the way?

@dcarpentiero
Copy link
Contributor Author

@subhramit I don't know I think I found it like this if I remember correctly. BTW i removed the "..."
tomorrow I will resolve the test issues, thanks for helping out

@subhramit
Copy link
Member

@subhramit I don't know I think I found it like this if I remember correctly. BTW i removed the "..."

Check the other PRs to see the unmodified version

@dcarpentiero
Copy link
Contributor Author

@subhramit I don't know I think I found it like this if I remember correctly. BTW i removed the "..."

Check the other PRs to see the unmodified version

Ohh thanks I see... I corrected it!


resultMap.put(entryType, new EntryTypeResult(uniqueFields, differingEntries));
Set<BibEntry> entries = entryTypeToEntriesMap.get(entryType);
if (entries == null || entries.size() <= 1 || differingFields.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

I think, the check is OK. However, I think, this case never happens. Therefore, I added an assert stgatement.

@koppor
Copy link
Member

koppor commented Aug 27, 2025

We use this PR to fix #13765

@subhramit subhramit added the status: awaiting-second-review For non-trivial changes label Aug 27, 2025
Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Lets get this merged.

@calixtus calixtus added this pull request to the merge queue Aug 27, 2025
Merged via the queue into JabRef:main with commit 9b2772b Aug 27, 2025
1 check passed
@koppor koppor mentioned this pull request Aug 27, 2025
1 task
}

entryTypeToEntriesMap
.computeIfAbsent(entryType, _ -> new java.util.LinkedHashSet<>())
Copy link
Member

@subhramit subhramit Aug 27, 2025

Choose a reason for hiding this comment

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

I think we could use a hash set here?
Also, should use import

Copy link
Member

Choose a reason for hiding this comment

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

Oh, yes. Follow-up? ^^

@koppor koppor mentioned this pull request Aug 27, 2025
1 task
@dcarpentiero dcarpentiero deleted the mode-aware-consistency-check-13467-clean branch August 28, 2025 07:15
@dcarpentiero dcarpentiero restored the mode-aware-consistency-check-13467-clean branch August 28, 2025 07:15
@dcarpentiero dcarpentiero deleted the mode-aware-consistency-check-13467-clean branch August 28, 2025 07:39
Siedlerchr added a commit that referenced this pull request Aug 28, 2025
…n-c23b1a88-5c29-46a2-a007-701edd344b9b' into jetbrains-junie-issue-13623-run-c23b1a88-5c29-46a2-a007-701edd344b9b

* upstream/jetbrains-junie-issue-13623-run-c23b1a88-5c29-46a2-a007-701edd344b9b:
  Streamline code for getTabTitle() (#13781)
  Add missing variables
  Reformat BibTex in Show BibTex Source (#13761)
  Auto publish before JBang tests (#13779)
  Hotfix: calling of publish.yml
  Support BibLaTeX datamodel validations (#13693)
  Auto add and remove of "status: changes-required" label (#13778)
  New Crowdin updates (#13777)
  Restore local-only Git behavior for SLR to fix repository initialization error (#13775)
  Use vanilla hashset (#13771)
  Fix Springer Fetcher names (#13770)
  Fix condition
  Publish SNAPSHOT on jablib change (#13774)
  Adapt as per new set of checks (#13772)
  Bump jablib/src/main/resources/csl-styles from `1194364` to `17cfa60` (#13750)
  Fix path (#13769)
  Mode aware consistency check (#13584)
  Refine JBang check (#13765)
  Add Language Server to the UI and add the integrity/consistency check (#13697)
  Fix/remove comment code (#13763)
Siedlerchr added a commit that referenced this pull request Sep 8, 2025
* upstream/main: (32 commits)
  Fix path (#13769)
  Mode aware consistency check (#13584)
  Refine JBang check (#13765)
  Add Language Server to the UI and add the integrity/consistency check (#13697)
  Fix/remove comment code (#13763)
  New Crowdin updates (#13760)
  Bump org.openrewrite.rewrite from 7.14.0 to 7.14.1 (#13757)
  Bump com.autonomousapps:dependency-analysis-gradle-plugin (#13756)
  Bump dev.langchain4j:langchain4j-bom from 1.2.0 to 1.3.0 in /versions (#13755)
  Bump jablib/src/main/resources/csl-locales from `fa56de1` to `e29c453` (#13754)
  Bump com.autonomousapps:dependency-analysis-gradle-plugin (#13753)
  Bump org.mockito:mockito-core from 5.18.0 to 5.19.0 in /versions (#13752)
  Bump actions/upload-pages-artifact from 3 to 4 (#13751)
  Migrate fetchers to Search.g4 ANTLR parser. (#13691)
  [Junie]: fix: resolve IllegalArgumentException for non-absolute URIs (#13669)
  Add auto-renaming of linked files on entry data change (#13295)
  Walkthrough additions (#13745)
  Switch from zulu to corretto (#13749)
  New Crowdin updates (#13747)
  Fix copy to (#13741)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mode-aware consistency check

5 participants