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 Generator for Bindable Property #1321

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

pictos
Copy link
Member

@pictos pictos commented Aug 7, 2023

This will be used by the lib only, it shouldn't be part of our public nuget, because of that I deleted the template...

I'll point out the code areas that, I believe, can cause questions. In the future should be easier to port the code to the public SG project and delete this one

Fixes #542

SyntaxPredicate, SemanticTransform)
.Where(static x => x.ClassInformation != default || !x.BPInfos.IsEmpty)
.Collect()
.SelectMany(static (types, _) => types);
Copy link
Member Author

Choose a reason for hiding this comment

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

This was necessary to avoid the provider to be a collection. I prefer to do it here instead of having a foreach loop inside the Execute method

void Execute(SourceProductionContext context, SemanticValues semanticValues)
{
var source = GenerateSource(semanticValues);
SourceStringService.FormatText(ref source);
Copy link
Member Author

Choose a reason for hiding this comment

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

I copied the code as a reference, so there's no code duplication. When this goes to the main project nothing will change

{
var sb = new StringBuilder($@"
// <auto-generated>
// Test2 : {DateTime.Now}
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll leave this here, this will allow us to see if the incremental behavior is working. The value should change only when we edit/add/remove the BindablePropertyAttribute, if it changes in any other scenario that could be a bug.

Comment on lines 89 to 91
/// <summary>
/// Backing BindableProperty for the <see cref="PropertyName"/> property.
/// </summary>
Copy link
Member Author

Choose a reason for hiding this comment

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

For us this will work very well, not sure how to improve this when it goes public.

Comment on lines 117 to 118
/// <inheritdoc />
sb.AppendLine("/// <inheritdoc />");
Copy link
Member Author

Choose a reason for hiding this comment

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

For us this will work very well, not sure how to improve this when it goes public. This is inherit by the inline docs on the interface

@@ -0,0 +1,200 @@
//https://github.com/CommunityToolkit/dotnet/blob/main/src/CommunityToolkit.Mvvm.SourceGenerators/Helpers/EquatableArray%7BT%7D.cs
Copy link
Member Author

Choose a reason for hiding this comment

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

We need this because we will work with a collection of BindableProperties... And all the other collections didn't work because they aren't very good with Equals, so I grabbed this one from Sergio, and works like a charm.

@@ -0,0 +1,188 @@
// https://github.com/CommunityToolkit/dotnet/blob/main/src/CommunityToolkit.Mvvm.SourceGenerators/Helpers/HashCode.cs
Copy link
Member Author

Choose a reason for hiding this comment

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

Again I grabbed some code from Sergio, this is used by the EquatableArray<T>, and it's necessary because we are on .netstandard. And because of this puppy we have to allow unsafe code

Yeah, I know... We are limited by the technology of our era


namespace CommunityToolkit.Maui.BindablePropertySG.Models;

record BPInfo
Copy link
Member Author

Choose a reason for hiding this comment

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

The IsExternalInit class is used on our regular SG to allow the usage of records that are great for keeping the incremental behavior. I didn't added as link because VS complained sometimes

Comment on lines +10 to +13
[BindableProperty<IView>("Header", PropertyChangedMethodName = nameof(OnHeaderPropertyChanged))]
[BindableProperty<IView>("Content", PropertyChangedMethodName = nameof(OnContentPropertyChanged))]
[BindableProperty<bool>("IsExpanded", PropertyChangedMethodName = nameof(OnIsExpandedPropertyChanged))]
[BindableProperty<object>("CommandParameter")]
[BindableProperty<ICommand>("Command")]
Copy link
Member Author

Choose a reason for hiding this comment

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

Here you've an example of usage

Copy link
Collaborator

Choose a reason for hiding this comment

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

I love those generic attributes! 😍

get => (bool)GetValue(IsExpandedProperty);
set => SetValue(IsExpandedProperty, value);
}

/// <inheritdoc />
Copy link
Member Author

Choose a reason for hiding this comment

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

Since Direction has a custom set method I don't use the SG to generate it. Maybe in the future, when we have partial Property we can come back here and improve it

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't a good idea to cast all Enums to int, some enums don't have a int value or use the Flag attribute. Here I would say it's best to let devs handle that instead of forcing something

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry, accidentally deleted comment:
We can choose another more generic exception (or even create a custom) or just not cast to int.

if (enum)
{
	public PropertyEnumType PropertyName
	{
		get => (PropertyEnumType)GetValue(PropertyNameProperty);
		set
		{
			if (!Enum.IsDefined(typeof(PropertyEnumType), value))
			{
				throw new InvalidEnumException(nameof(value), value, typeof(PropertyEnumType));
			}

			SetValue(PropertyNameProperty, value);
		}
	}
}

Or we can have partial method OnPropertyChanging(), where developer can add additional logic. It will be even more flexible:

public PropertyType PropertyName
	{
		get => (PropertyType)GetValue(PropertyNameProperty);
		set
		{
			OnPropertyChanging(value);
			SetValue(PropertyNameProperty, value);
		}
	}

Copy link
Member Author

Choose a reason for hiding this comment

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

With that, we will infer a lot about enums used on SG, in this case I think it's better to leave at it's and measure that on our usage

{
var data = attribute.ConstructorArguments[0];

return data.Value is null ? throw new NullReferenceException() : data.Value.ToString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we add a message for NRE describing what is null?

@brminnick
Copy link
Collaborator

brminnick commented Aug 12, 2023

Thanks Pedro! Just FYI - I renamed the new class and its namespace to CommunityToolkit.Maui.SourceGenerators.Internal.

This opens up the opportunity for us to use this csproj for additional internal source generators in the future.

And let's convert all of our BindablePropertys in this PR before we merge. I'm happy to help convert them all.

@brminnick
Copy link
Collaborator

brminnick commented Aug 12, 2023

Hey Pedro! This is looking really really cool!

Could you add the ability to include XML comments in the attribute? Right now <inheritdoc /> is hardcoded, but many of our already-implemented properties have customized XML docs. I tried to convert a couple existing BindablePropertys, but ran into this as a blocker.

👇 Maybe it'd look something like this?

[BindableProperty<bool>("IsExpanded", PropertyChangedMethodName = nameof(OnIsExpandedPropertyChanged), XMLComment="<inheritdoc />")]

@pictos
Copy link
Member Author

pictos commented Aug 12, 2023

@brminnick I didn't have a good approach for XML docs, because they can be small or big, and not sure how that will fit with SG... Maybe let this be solved when C# has partial properties?

OF course, you may have a bunch of const string to reference, but I don't feel it very idiomatic

Comment on lines +6 to +8
record BindablePropertyModel
{
public string PropertyName { get; set; } = string.Empty;
Copy link
Collaborator

@brminnick brminnick Aug 14, 2023

Choose a reason for hiding this comment

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

1. Add record XMLTagModel

I suggest we create a new record XMLTagModel and add it to BindablePropertyModel as a nullable object. If BindablePropertyModel.XMLTags is null, then the source generator won't add any XML comments. If it is instantiated, then the source generator will generate the XML documentation for the non-null strings in XMLTagModel.

The nullable string ensures we don't accidentally generate a blank XML comment for Summary, for example.

record XMLTagModel(string? Summary, string? Remarks, string? Returns, string? Param, string? Paramref, string? ExceptionDescription);

record BindablePropertyModel
{
        public XMLTagModel? XMLTags { get; set; }
	public string PropertyName { get; set; } = string.Empty;
// ...
}

2. Add XML Tag string?s to BindablePropertyModel

If using an object in the attribute doesn't look/feel right, we can add each XML Tag option to BindablePropertyModel as a nullable string.

The nullable string ensures we don't accidentally generate a blank XML comment for Summary, for example.

record BindablePropertyModel
{
	public string? Summary { get; set; }
	public string? Remarks { get; set; }
	public string? Returns { get; set; }
	public string? Param { get; set; }
	public string? Paramref { get; set; }
	public string? ExceptionDescription { get; set; }
	public string PropertyName { get; set; } = string.Empty;
// ...
}

Then the source generators can add XML documentation for the generated properties.

If our BindableProperty Source Generator doesn't support XML tags, then we can't use it in the Toolkit because we have so many BindableProperties with unique XML tags.

Copy link
Member Author

Choose a reason for hiding this comment

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

For 1:
The usage of a custom type will not work, because it will not be a valid type for attribute, as you can see here.

For 2:

That will cause a lot of noise on the attribute, making it very big. Don't think the best solution will be using C#, I know that SG can consume xml and json files. The usage of xml files for docs is already used on Essentials and Maui as well, maybe we can mimic that with SG?

@ghost ghost added stale The author has not responded in over 30 days help wanted This proposal has been approved and is ready to be implemented labels Sep 27, 2023
@VladislavAntonyuk
Copy link
Collaborator

@pictos do we need any unresolved issues or open questions? Can we merge this PR as it is?

{
var sb = new StringBuilder($@"
// <auto-generated>
// Test2 : {DateTime.Now}
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

for now, no... This is to make sure our generator is incremental. We can remove it when this goes public

record BindablePropertyModel
{
public string PropertyName { get; set; } = string.Empty;
public ITypeSymbol? ReturnType { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need the whole ITypeSymbol? can we extract only necessary properties?

@listepo
Copy link

listepo commented Apr 14, 2024

any news?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted This proposal has been approved and is ready to be implemented stale The author has not responded in over 30 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Proposal] BindableProperty generator
4 participants