-
Notifications
You must be signed in to change notification settings - Fork 23
Fix case when not properly considering missing xmp entry #721
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
Conversation
WalkthroughThe change adjusts null-handling in two date-comparison methods in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
validation-model/src/main/java/org/verapdf/gf/model/impl/cos/GFCosInfo.java (1)
234-241: Mod date logic consistent with creation date; consider small refactorThe modification-date logic mirrors the creation-date behavior (null only when trailer
ModDateis missing, false when trailer date is present but XMP modify date is missing), which keeps things consistent and fixes the missing-XMP case.Given the near-duplication between
getdoCreationDatesMatchandgetdoModDatesMatch, you could optionally extract a small private helper that takes the trailer date string and a supplier of the corresponding XMPCalendar, to reduce repetition and keep the matching semantics in one place. Not required, but would tighten maintainability if these rules evolve further.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
validation-model/src/main/java/org/verapdf/gf/model/impl/cos/GFCosInfo.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Quality Assurance
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
validation-model/src/main/java/org/verapdf/gf/model/impl/cos/GFCosInfo.java (1)
214-221: Creation date tri‑state behavior change looks correct; confirm asymmetric handling is intendedThe new logic (return
nullonly whencreationDateis missing, andfalsewhencreationDateis present but XMP create date is missing) matches the PR goal and avoids silently returningnullin the “missing XMP entry” case. This will change rule outcomes for files where only the trailer creation date is present.One thing to double‑check: when XMP create date exists but trailer
CreationDateis missing, the method still returnsnullrather thanfalse. If the intended semantics are “PDF date present but no XMP → mismatch (false)” and “XMP present but no PDF → also mismatch”, you might want to mirror that here; if not, the current asymmetric behavior is fine but should be deliberate.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.