-
-
Notifications
You must be signed in to change notification settings - Fork 8
fix: IgnoreComparer should check for CompareDcision.Difference #56
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
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.
Pull request overview
This PR enhances the HTML comparison engine to properly handle combined comparison decisions using flag-based checks. The core issue being addressed is that when CompareDecision enum has combined flag values (like "Different AND SkipChildren"), the code needs to use HasFlag() rather than equality checks to detect these combinations correctly.
Key Changes:
- Added three new combined enum values to
CompareDecision:DifferentAndSkipChildren,DifferentAndSkipAttributes, andDifferentAndSkipChildrenAndSkipAttributes - Updated comparison logic to use
HasFlag()instead of equality checks where appropriate - Added corresponding static
CompareResultinstances for the new combined decisions
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
CompareDecision.cs |
Adds three new combined flag enum values to represent scenarios where elements are different but certain comparisons should be skipped |
CompareResult.cs |
Adds static instances for the new combined decisions; updates IsSameOrSkip property to use HasFlag() for proper flag checking; fixes documentation to reference CompareDecision instead of CompareResult |
IgnoreChildrenElementComparer.cs |
Updates early-return check to use HasFlag(); adds switch cases to handle Different and Different|SkipAttributes decisions by returning appropriate combined skip results |
IgnoreAttributesElementComparer.cs |
Adds switch cases to handle Different and DifferentAndSkipChildren decisions by returning appropriate combined skip results |
HtmlDifferenceEngine.cs |
Updates attribute comparison to use HasFlag(CompareDecision.Different) instead of equality check, ensuring it detects differences in combined flag values |
IgnoreChildrenElementComparerTest.cs |
Adds Test004 verifying Different → DifferentAndSkipChildren transformation; adds Test005 verifying DifferentAndSkipAttributes → DifferentAndSkipChildrenAndSkipAttributes transformation |
IgnoreAttributesElementComparerTest.cs |
Adds Test004 verifying Different → DifferentAndSkipAttributes transformation; adds Test005 verifying DifferentAndSkipChildren → DifferentAndSkipChildrenAndSkipAttributes transformation |
egil
left a 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.
Changed a few other places where HasFlag should be used to check existing compare decision
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.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
| CompareDecision.Same => CompareResult.SkipChildren, | ||
| CompareDecision.Different => CompareResult.SkipChildren, | ||
| CompareDecision.Different => CompareResult.DifferentAndSkipChildren, | ||
| CompareDecision.Different | CompareDecision.SkipAttributes => CompareResult.DifferentAndSkipChildrenAndSkipAttributes, |
Copilot
AI
Dec 6, 2025
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.
[nitpick] Consider using the named enum member CompareDecision.DifferentAndSkipAttributes instead of the inline bitwise OR pattern CompareDecision.Different | CompareDecision.SkipAttributes for consistency with line 27 in IgnoreAttributesElementComparer.cs, which uses CompareDecision.DifferentAndSkipChildren. This would improve code clarity and maintainability.
| CompareDecision.Different | CompareDecision.SkipAttributes => CompareResult.DifferentAndSkipChildrenAndSkipAttributes, | |
| CompareDecision.DifferentAndSkipAttributes => CompareResult.DifferentAndSkipChildrenAndSkipAttributes, |
@egil I do think I catched at least one other spot.