-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Fix: fallback conversion from BibTeX year field to biblatex date field #13040
Conversation
…dates in BibTeX `year` field
src/main/java/org/jabref/logic/cleanup/ConvertToBiblatexCleanup.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/cleanup/ConvertToBiblatexCleanup.java
Outdated
Show resolved
Hide resolved
…dates in BibTeX `year` field
…onvert-to-biblatex
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.
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) |
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.
Concise changelog entries are preferred.
- 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(); |
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.
trim was already used when obtaining yearValue
, so this one seems redundant.
…ConvertToBiblatexCleanup
return changes; | ||
} | ||
|
||
private Collection<? extends FieldChange> applyDateFallback(BibEntry entry) { |
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.
Oh, another thing -- can we use more specificity here?
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).
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.
My bad! I used IntelliJ’s autocomplete but didn’t pay enough attention to the return type 😭. Will change it to List, thanks!
jablib/src/main/java/org/jabref/logic/cleanup/ConvertToBiblatexCleanup.java
Outdated
Show resolved
Hide resolved
@@ -53,6 +54,27 @@ public List<FieldChange> cleanup(BibEntry entry) { | |||
} | |||
}); | |||
} | |||
// If still no 'date' field, try fallback logic | |||
if (entry.getField(StandardField.DATE).isEmpty()) { |
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.
Don't we have "hasField"?
Also later instead of trim and is empty maybe just hasField?
The parser should deal with empty strings IMHO
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 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!
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.
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
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.
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? 😅
…onvert-to-biblatex
return changes; | ||
} | ||
|
||
entry.setField(StandardField.DATE, yearText).ifPresent(changes::add); |
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 method setField should use withField for creating a new BibEntry object, as per JabRef's guidelines for creating new objects.
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.
Here, it is OK.
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 small Java code style nitpicks - just to share knowledge about JabRef's coding conventions. - Hope, this is OK. 😅
Optional<Date> fallbackDate = Date.parse(yearText); | ||
if (fallbackDate.isEmpty()) { |
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, 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); |
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.
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<>(); |
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 would move that down before line 77
|
||
Optional<String> yearValue = entry.getFieldOrAlias(StandardField.YEAR).map(String::trim); | ||
if (yearValue.isEmpty()) { | ||
return changes; |
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.
Return List.of()
String yearText = yearValue.get(); | ||
Optional<Date> fallbackDate = Date.parse(yearText); | ||
if (fallbackDate.isEmpty()) { | ||
return changes; |
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.
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. |
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 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.
@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 :) |
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?
This fallback is intentionally conservative and only runs if the standard cleanup logic does not produce a date.
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)