-
Notifications
You must be signed in to change notification settings - Fork 11
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
Refactored SampleGen test tooling #69
Conversation
…for when executing in tests
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.
Small request to update to new string literal. Otherwise, seems alright to me so far. @Arlodotexe did you test updating the main repo to this branch and seeing that it builds as expected there with these changes?
Wouldn't mind a quick pass from @Sergio0694 as well as our resident generator expert... 😉
CommunityToolkit.Tooling.SampleGen.Tests/ToolkitSampleGeneratedPaneTests.cs
Outdated
Show resolved
Hide resolved
CommunityToolkit.Tooling.SampleGen/Metadata/ToolkitSampleButtonCommand.cs
Show resolved
Hide resolved
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.
Had a look at the generator, changes seem fine to me under the assumption that the generator was already non-incremental for most of its pipeline and generally incredibly expensive, but my understanding is this intended 😆
CommunityToolkit.Tooling.SampleGen/ToolkitSampleMetadataGenerator.Sample.cs
Outdated
Show resolved
Hide resolved
CommunityToolkit.Tooling.SampleGen/ToolkitSampleMetadataGenerator.Sample.cs
Show resolved
Hide resolved
CommunityToolkit.Tooling.SampleGen/ToolkitSampleOptionGenerator.cs
Outdated
Show resolved
Hide resolved
…r.cs Co-authored-by: Sergio Pedri <[email protected]>
…tor.Sample.cs Co-authored-by: Sergio Pedri <[email protected]>
@Arlodotexe LGTM, but looks like conflicts with the bast branch now to resolve? |
Had a conflict on pull at one point, so this branch contains a merge commit. Rebase has conflicts, but Squash or Merge will work fine. I'd like to be able to revert 102f46a in the future to continue that work, so I'll use a normal merge here. |
Until recently, tests only checked the diagnostic output of a source generator, not the actual generated code. This changed with CommunityToolkit/Labs-Windows#336 when we added the
PaneOption_GeneratesTitleProperty
test.This PR contains additional improvements extracted from #44.