Skip to content
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

Generate AngleSharp based on included AngleSharp version #1286

Merged
merged 15 commits into from
Nov 23, 2023
Merged

Conversation

egil
Copy link
Member

@egil egil commented Nov 20, 2023

Pull request description

This replaces AngleSharpWrappers with similar types that are generated using a source generator project. This allows us to easily upgrade AngleSharp and have new public members supported by the wrappers.

This uses Verify to snapshot test the generated wrappers.

This also upgrades AngleSharp to latest versions. Eventhough AngleSharp.Css I a preview release, it should be safe to use, according to AngleSharp/AngleSharp.Css#150.

The downside is that the generators adds a little to the build time. They should only run one time, as far as I can tell, but we may need to optimize on this.

It also changes the IElementFactory type and makes the wrappers easier to work with, something relevant with #1252.

@linkdotnet
Copy link
Collaborator

Samples and or docs probably have to be adapted as well

@egil
Copy link
Member Author

egil commented Nov 20, 2023

Samples and or docs probably have to be adapted as well

working on it. docs was not set to build against .net 8.

@egil
Copy link
Member Author

egil commented Nov 22, 2023

@linkdotnet improved the generator quite a bit. Want to take a second look at it?

@linkdotnet
Copy link
Collaborator

@linkdotnet improved the generator quite a bit. Want to take a second look at it?

Will check it out tomorrow and fetch the branch to play aroud

@egil
Copy link
Member Author

egil commented Nov 22, 2023

There is technically a breaking change. The old wrapper types were public, the new ones are internal. I am tempted to disregard this as I've always considered the wrappers a pubternal thing. Whats your take?

@linkdotnet
Copy link
Collaborator

There is technically a breaking change. The old wrapper types were public, the new ones are internal. I am tempted to disregard this as I've always considered the wrappers a pubternal thing. Whats your take?

Have a similar opinion - while they were public, we never directly exposed them. The code always returned an IElement.

@egil egil requested a review from linkdotnet November 23, 2023 15:17

var elementInterfacetypes = FindElementInterfaces(angleSharpAssembly);

var source = new StringBuilder();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small perf optimization: Initialize with a pre-allocated buffer.

Suggested change
var source = new StringBuilder();
var source = new StringBuilder(2000);

@linkdotnet
Copy link
Collaborator

Really nice work - an initial build is "noticeable" slower now - but I am not sure if there as much you can do.

public TElement WrappedElement
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get
Copy link
Collaborator

Choose a reason for hiding this comment

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

The whole get can be simplfiied to:

		get => element ??= elementFactory.GetElement<TElement>();

@egil egil merged commit d360563 into main Nov 23, 2023
4 checks passed
@egil egil deleted the anglesharp-gen branch November 23, 2023 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants