Skip to content

Fix: fallback conversion from BibTeX year field to biblatex date field #13040

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

Merged
merged 13 commits into from
May 4, 2025

Conversation

wanling0000
Copy link
Contributor

@wanling0000 wanling0000 commented Apr 30, 2025

Closes #11868
This PR introduces a fallback mechanism in the "Convert to biblatex" cleanup step to improve compatibility with BibLaTeX-style date formats mistakenly entered in the BibTeX year field.

What was the problem?
Users reported that when year = {2011-11-11} (or other extended formats) is used in BibTeX mode, the "Convert to biblatex" cleanup did not populate the date field as expected.

This is because the cleanup logic only merges year + month if both are present and parsable. Non-standard values in year were silently ignored.

What has been changed?

  • A fallback step was added after the main conversion logic:
    • If date is still missing, and
    • year contains a parsable BibLaTeX-style date,
    • then date is set directly from year, and year is cleared.

This fallback is intentionally conservative and only runs if the standard cleanup logic does not produce a 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.

@wanling0000 wanling0000 marked this pull request as ready for review April 30, 2025 22:45
Copy link
Member

@subhramit subhramit left a comment

Choose a reason for hiding this comment

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

Hi, thanks for taking this up.
PR looks good to me, just some nits.

CHANGELOG.md Outdated
@@ -15,6 +15,8 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv

### Fixed

- We fixed an issue where the "Convert to biblatex" cleanup failed to populate the `date` field if `year` contained a full date in ISO format (e.g., `2011-11-11`). A fallback was added to correctly interpret such values. [#11868](https://github.com/JabRef/jabref/issues/11868)
Copy link
Member

@subhramit subhramit May 1, 2025

Choose a reason for hiding this comment

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

Concise changelog entries are preferred.

Suggested change
- We fixed an issue where the "Convert to biblatex" cleanup failed to populate the `date` field if `year` contained a full date in ISO format (e.g., `2011-11-11`). A fallback was added to correctly interpret such values. [#11868](https://github.com/JabRef/jabref/issues/11868)
- We added a fallback for the "Convert to biblatex" cleanup when it failed to populate the `date` field if `year` contained a full date in ISO format (e.g., `2011-11-11`). [#11868](https://github.com/JabRef/jabref/issues/11868)

Optional<String> monthValue = entry.getFieldOrAlias(StandardField.MONTH).map(String::trim);

if (yearValue.isPresent() && monthValue.isEmpty()) {
String yearText = yearValue.get().trim();
Copy link
Member

Choose a reason for hiding this comment

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

trim was already used when obtaining yearValue, so this one seems redundant.

return changes;
}

private Collection<? extends FieldChange> applyDateFallback(BibEntry entry) {
Copy link
Member

Choose a reason for hiding this comment

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

Oh, another thing -- can we use more specificity here?

Suggested change
private Collection<? extends FieldChange> applyDateFallback(BibEntry entry) {
private List<FieldChange> applyDateFallback(BibEntry entry) {

Or was there a reason you chose Collection<? extends FieldChange>?

(In case you go with this suggestion, remove the unused Collection import too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad! I used IntelliJ’s autocomplete but didn’t pay enough attention to the return type 😭. Will change it to List, thanks!

subhramit
subhramit previously approved these changes May 1, 2025
@@ -53,6 +54,27 @@ public List<FieldChange> cleanup(BibEntry entry) {
}
});
}
// If still no 'date' field, try fallback logic
if (entry.getField(StandardField.DATE).isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

Don't we have "hasField"?

Also later instead of trim and is empty maybe just hasField?

The parser should deal with empty strings IMHO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I applied your suggestion to use .hasField for the conditional check, thanks for pointing that out!

However, I kept the trim call inside the method body. I added a test which shows that Date.parse() does not consistently handle trimming of input. While the UI typically strips whitespace automatically, I added this for robustness in case the input comes from other sources.

I'm happy to remove the test if you think it's unnecessary, just let me know!

Copy link
Member

Choose a reason for hiding this comment

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

If we are testing for a negative, I think we should have an assertNotEquals case without the cleanup and an assertEquals after the cleaup.
Along with that, add a comment above the test:

Date.parse() does not consistently handle trimming of input. While the UI typically strips whitespace automatically

To elaborate its purpose

Copy link
Member

Choose a reason for hiding this comment

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

However, I kept the trim call inside the method body. I added a test which shows that Date.parse() does not consistently handle trimming of input

Why not adding trim functionality and tests to the parsing? 😅

@subhramit subhramit added status: changes-required Pull requests that are not yet complete and removed status: awaiting-second-review For non-trivial changes labels May 1, 2025
return changes;
}

entry.setField(StandardField.DATE, yearText).ifPresent(changes::add);
Copy link

Choose a reason for hiding this comment

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

The method setField should use withField for creating a new BibEntry object, as per JabRef's guidelines for creating new objects.

Copy link
Member

Choose a reason for hiding this comment

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

Here, it is OK.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Some small Java code style nitpicks - just to share knowledge about JabRef's coding conventions. - Hope, this is OK. 😅

Comment on lines 73 to 74
Optional<Date> fallbackDate = Date.parse(yearText);
if (fallbackDate.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 variable fallbackDate is only used at the if -- maybe, inline and say as java comment

// check if yearText is a valid date

return changes;
}

entry.setField(StandardField.DATE, yearText).ifPresent(changes::add);
Copy link
Member

Choose a reason for hiding this comment

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

Add before as java comment:

// If year was a full date, move it from year to date field.

}

private List<FieldChange> applyDateFallback(BibEntry entry) {
List<FieldChange> changes = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

I would move that down before line 77


Optional<String> yearValue = entry.getFieldOrAlias(StandardField.YEAR).map(String::trim);
if (yearValue.isEmpty()) {
return changes;
Copy link
Member

Choose a reason for hiding this comment

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

Return List.of()

String yearText = yearValue.get();
Optional<Date> fallbackDate = Date.parse(yearText);
if (fallbackDate.isEmpty()) {
return changes;
Copy link
Member

Choose a reason for hiding this comment

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

Return List.of()

@@ -123,4 +123,13 @@ void parseZonedTime() {
void parseDateNull() {
assertThrows(NullPointerException.class, () -> Date.parse(null));
}

// Date.parse() has been updated to defensively strip surrounding whitespace from input strings.
Copy link

Choose a reason for hiding this comment

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

The comment is trivial and restates the obvious, which violates the guideline that comments should add new information and not be plainly derived from the code itself.

@koppor koppor added this pull request to the merge queue May 4, 2025
@koppor
Copy link
Member

koppor commented May 4, 2025

@wanling0000 Thank you for the clean code and the efforts to reach it!

Merged via the queue into JabRef:main with commit 99d49ca May 4, 2025
1 check passed
@wanling0000
Copy link
Contributor Author

@wanling0000 Thank you for the clean code and the efforts to reach it!

Thanks! I’ve actually learned quite a bit from this review, appreciate all the suggestions :)

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.

Cleanup entries - Convert to biblatex does not work
3 participants