Skip to content

Conversation

RiRiSharp
Copy link
Contributor

@RiRiSharp RiRiSharp commented Sep 11, 2025

Types of Changes

Prerequisites

Please make sure you can check the following two boxes:

  • I have read the CONTRIBUTING document
  • My code follows the code style of this project

Contribution Type

What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue, please reference the issue id)
  • New feature (non-breaking change which adds functionality, make sure to open an associated issue first)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Description

Closes #48

@CLAassistant
Copy link

CLAassistant commented Sep 11, 2025

CLA assistant check
All committers have signed the CLA.

@RiRiSharp RiRiSharp changed the title Ignore unmatched :ignore attributes Ignore unmatched :ignore attributes Sep 11, 2025
@RiRiSharp
Copy link
Contributor Author

Let me know what you think, I loved how everything is set up, code looks very clean. But maybe my changes could be better placed somewhere else, let me know!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe revert this change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or maybe merge the two files into one static file, e.g. IgnoreAttributeStrategy.cs, have the Compare and Match methods in the static class, and then have the static IsIgnoreAttribute method there being shared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to keep IgnoreAttributeComparer for backwards compatibility?

@egil egil requested a review from Copilot September 14, 2025 18:06
Copy link

@Copilot Copilot AI left a 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 fixes a bug where unmatched :ignore attributes were not being properly handled by the diffing engine. Previously, :ignore attributes that didn't have corresponding attributes in the test HTML would still cause diff failures instead of being ignored as intended.

  • Added a new IgnoreAttributeMatcher to handle unmatched :ignore attributes by matching them with themselves
  • Refactored IgnoreAttributeComparer to extract ignore attribute detection logic into a reusable method
  • Added comprehensive test coverage for various scenarios with :ignore attributes

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
IgnoreAttributeMatcher.cs New matcher strategy that handles unmatched :ignore attributes by self-matching
IgnoreAttributeComparer.cs Refactored to extract ignore attribute detection into a reusable method
DiffingStrategyPipelineBuilderExtensions.cs Integrated the new ignore attribute matcher into the pipeline
SourceMap.cs Updated to use extracted method for unmatched attribute detection
HtmlDifferenceEngine.cs Added necessary using statement for attribute strategies
IgnoreAttributeTestData.cs New test data class with comprehensive test cases for ignore attribute scenarios
DiffBuilderTest.cs Added test methods to verify ignore attribute functionality works correctly

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@RiRiSharp
Copy link
Contributor Author

RiRiSharp commented Sep 14, 2025

I have:

@egil
Copy link
Collaborator

egil commented Sep 15, 2025

Thanks, I think looks good.

Lets ignore the failing Linux build for now. Asked Copilot to change that in a separate PR.

This reverts commit 2597558.
@RiRiSharp
Copy link
Contributor Author

Lets ignore the failing Linux build for now. Asked Copilot to change that in a separate PR.
I have reverted that commit!

It was nice contributing! If there are ever open issues and you don't have time, let me know.

Copy link
Collaborator

@egil egil left a comment

Choose a reason for hiding this comment

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

Thanks, lets get this merged.

@egil egil merged commit 600656d into AngleSharp:devel Sep 15, 2025
1 check passed
@egil
Copy link
Collaborator

egil commented Sep 15, 2025

Lets ignore the failing Linux build for now. Asked Copilot to change that in a separate PR.
I have reverted that commit!

It was nice contributing! If there are ever open issues and you don't have time, let me know.

This repository does not have a lot. Feel free to look over the code base, see if there are things that needs updating, e.g. new html elements or Boolean attributes that are counted as boolean.

Otherwise, we would love help over in the bUnit repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make :ignore also ingore existance of an attribute altogether
3 participants