-
Notifications
You must be signed in to change notification settings - Fork 18
Add WCAG 2.2 PDF2.0 favours #686
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 changes introduce WCAG 2.2 PDF 2.0 flavour variants while refactoring WCAG-specific logic across multiple classes. Two new PDFFlavour constants are added, the Specification enum is updated to separate WCAG 2.2 from PDF 2.0 specs, and WCAG-specific conditional branches are removed from structure element and helper classes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Key areas requiring attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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)
src/main/java/org/verapdf/pd/structure/PDStructElem.java (1)
157-166: Simplify the assignment operator for clarity.On line 163, the
|=operator is misleading. SinceisStandardis initialized tofalseon line 159 and the if-else structure ensures only one branch executes,isStandardis alwaysfalsewhen reaching line 163. Therefore,isStandard |= TaggedPDFRoleMapHelper.isStandardType(type)is functionally equivalent toisStandard = TaggedPDFRoleMapHelper.isStandardType(type).Apply this diff to simplify the operator:
} else if (type != null) { - isStandard |= TaggedPDFRoleMapHelper.isStandardType(type); + isStandard = TaggedPDFRoleMapHelper.isStandardType(type); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/verapdf/parser/PDFFlavour.java(2 hunks)src/main/java/org/verapdf/pd/structure/PDStructElem.java(2 hunks)src/main/java/org/verapdf/tools/TaggedPDFHelper.java(0 hunks)src/main/java/org/verapdf/tools/TaggedPDFRoleMapHelper.java(0 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/org/verapdf/tools/TaggedPDFRoleMapHelper.java
- src/main/java/org/verapdf/tools/TaggedPDFHelper.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/org/verapdf/pd/structure/PDStructElem.java (1)
src/main/java/org/verapdf/tools/TaggedPDFRoleMapHelper.java (1)
TaggedPDFRoleMapHelper(32-115)
⏰ 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 (4)
src/main/java/org/verapdf/parser/PDFFlavour.java (3)
45-46: LGTM! New WCAG 2.2 PDF 2.0 flavours added correctly.The new flavours follow the established naming convention and are properly mapped to the new
WCAG_2_2_PDF_2_0specification.
79-79: LGTM! New specification correctly maps WCAG 2.2 to PDF 2.0.The new
WCAG_2_2_PDF_2_0specification is properly defined with the correct family and PDF specification.
78-78: Specification change intentionally splits WCAG 2.2 support across two flavors with backward-compatible migration path.The change shows:
WCAG_2_2enum now maps to PDF 1.7 (line 78)- New
WCAG_2_2_PDF_2_0enum added for PDF 2.0 (line 79)- New flavour constants
WCAG_2_2_PDF_2_0_MACHINEandWCAG_2_2_PDF_2_0_HUMANadded (lines 45-46)Verification findings:
- ✓ No direct code references to
WCAG_2_2enum constant found outsidePDFFlavour.java- ✓ Specification checks in
PDStructElem.javacorrectly route based onISO_32000_2_0, so both variants work correctly- ✓ No configuration files hard-code WCAG flavor selection
- ✓ Migration path provided for those needing PDF 2.0 validation
Remaining concern: This is a user-facing default change — applications explicitly selecting
WCAG_2_2flavor will now validate against PDF 1.7 instead of PDF 2.0. Verify that this aligns with WCAG 2.2 specification requirements and that users selecting the old behavior can switch toWCAG_2_2_PDF_2_0.src/main/java/org/verapdf/pd/structure/PDStructElem.java (1)
130-145: Verify the behavioral change for WCAG 2.2 PDF 2.0 documents—test coverage not found.No test coverage exists in the repository for the modified methods
getStructureTypeStandardStructureTypeorisStandardStructureType. The PR introduces a significant behavioral change: WCAG 2.2 PDF 2.0 documents (which map to ISO_32000_2_0) now follow the ISO path exclusively, skipping role map helper mapping.Additionally, line 162 uses the
|=operator in an else-if branch whereisStandardis always false, making it functionally equivalent to=. This suggests incomplete refactoring of prior WCAG-specific conditional branches.Ensure this behavioral change is intentional, tested separately, or covered by external test infrastructure, and verify that role map helper mapping is correctly skipped for PDF 2.0 documents.
Summary by CodeRabbit
New Features
Removals
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.