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

Source generated sample registration #22

Merged
merged 13 commits into from
Jan 21, 2022

Conversation

Arlodotexe
Copy link
Member

Overview

  • Closes Investigate non-reflection alternatives to current sample system #18
  • Adds a source generator - crawls for ToolkitSampleAttribute and generates a method that returns metadata for all found.
  • Removed reflection-based sample discovery - in favor of the source generated sample data.
  • Adds C# 10 - to CommunityToolkit.Labs.Core only.
  • Adds IsExternalInit - Polyfill was needed to use Records.
  • Has been tested - all project heads have been tested and work with the new code.

Functional example

With temporary second sample control added

Attributes

image
image

Generated code

image

Full circle

image

@Arlodotexe Arlodotexe added the enhancement Improvement to an existing feature label Jan 5, 2022
@michael-hawker
Copy link
Member

michael-hawker commented Jan 14, 2022

UWP compile issue: MainPage.xaml.cs:77 selectedMetadata.Type wasn't updated to SampleControlType?

Should be:

var controlInstance = Activator.CreateInstance(selectedMetadata.SampleControlType);

In CommunityToolkit.Labs.Uwp. didn't see file changed in PR so I couldn't make a suggestion.

Otherwise, I think this is pretty cool, and I think good to go! Assuming this is Source Generator V2 vs V1?

I also know you wanted @Sergio0694 to take a quick look too for his input as the Source Generator ninja... 😋

@Arlodotexe
Copy link
Member Author

@michael-hawker Good catch, looks like merged code that didn't get updated. Build validation would be nice here haha.

@Sergio0694 Any feedback at all? I could probably make the source generator incremental but this is a pretty slim generator.

@michael-hawker
Copy link
Member

@Arlodotexe I know the main thing with the original source generators is that it doesn't matter if it doesn't do a lot of work if the solution/project is large enough it's still trying to run it on everything all the time. But maybe we won't have a big problem in Labs?

@Arlodotexe
Copy link
Member Author

@Arlodotexe I know the main thing with the original source generators is that it doesn't matter if it doesn't do a lot of work if the solution/project is large enough it's still trying to run it on everything all the time. But maybe we won't have a big problem in Labs?

I had an incremental version at one point, but it was difficult to debug so I swapped to the regular ones. Code should be clean enough to make switching pretty painless if we start seeing issues.

Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

The generator seems fine for what it needs to do, other than it not being incremental. Left a few notes here and there with some questions, but overall it looks ok at least for now. Note for the future, please consider documenting your code better 😅

Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

Seems fine, just left another couple of nits 👍

@Arlodotexe Arlodotexe merged commit 417937b into main Jan 21, 2022
@Arlodotexe Arlodotexe deleted the feature/source-generated-sample-registration branch January 21, 2022 21:29
Martin1994 pushed a commit to Martin1994/Labs-Windows that referenced this pull request Sep 2, 2023
…urce-generated-sample-registration

Source generated sample registration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate non-reflection alternatives to current sample system
3 participants