MarimerLLC / csla

A home for your business logic in any .NET application.
https://cslanet.com
MIT License
1.26k stars 403 forks source link

Create source generator for business class properties #4098

Closed rockfordlhotka closed 3 months ago

rockfordlhotka commented 4 months ago

My thought in creating this issue is to first have a discussion about how such a generator should work, and the scope it should cover.

@rockfordlhotka you gave me an amazing idea

after this issue I could create a source generator just like this

public partial class Test
{ 
    [AutoPropertyInfo]
    private string _city;

}

and generate this code

public partial class Test
{
  public Test()
  {
      _city = CityProperty.DefaultValue;
  }

  public static readonly PropertyInfo<string> CityProperty = RegisterProperty<string>(nameof(City), RelationshipTypes.PrivateField);
  public string City
    {
        get => GetProperty(CityProperty, _city);
        set => SetProperty(CityProperty, ref _city, value);
    }
}

this solves the point that I was looking for a way to remove all this extra code and also control the result class If you like the idea you can create the issue and assign to me.

Yep. Was also looking how to get rid of that boilerplate. I'll probably fiddle with that and write one (in my repo). I do hope the source gens will be released independent of the main csla stuff. I think they need to have a quicker release cycle when we really start supporting and using source generators.

Originally posted by @StefanOssendorf in https://github.com/MarimerLLC/csla/issues/2334#issuecomment-2225601699

luizfbicalho commented 4 months ago

I'm interested in this issue, after all it's my idea. What questions do you have about this?

rockfordlhotka commented 4 months ago

A couple thoughts.

First, when using private fields, a class must implement the IMobileObject interface methods to set/get the state of those private fields so serialization works. The generator should generate those methods as well (while still honoring any manual implementation of those methods).

Second, the AutoProperty attribute needs the ability to handle all the different property scenarios:

Third, the AutoProperty attribute needs to work based on the base class (keeping in mind that poeple often add custom base types into the inheritance tree, so we need to find the base class anywhere in the tree):

Fourth, another variation on the theme is that a string property can be backed by a SmartDate value. In this case, the field would be of type SmartDate, and the property would be of type string. However, it is also possible for someone to directly expose the SmartDate value as the property type - so both scenarios need to be available.

Fifth, the way you implement a non-serializable property is by using a private backing field and not including that field value in the get/set state methods of IMobileObject. The generator can probably key off the NonSerializable attribute if it is attached to the field, so we don't need a new attribute of our own.

rockfordlhotka commented 4 months ago

I'm envisioning something like this:

public class PersonEdit : BusinessBase<PersonEdit>
{
  [AutoProperty(PropertyName: "Name")]
  private string _name;

  [AutoProperty(PropertyName: "Name", PrivateSetter: true)]
  private string _name;

  [AutoProperty(PropertyName: "Name", PropertyScope: true)]
  private string _name;

  [AutoProperty(PropertyName: "Name"]
  [NonSerialized]
  private string _name;
}
rockfordlhotka commented 4 months ago

It occurs to me that we also need a way to set (and localize) the FriendlyName of the property meta-state in the generated code.

kant2002 commented 4 months ago

Isn't at this point it become almost the same as writing the code yourself? Maybe source gen should support only most common scenarios, and if somebody want all customization it just wrote code in old-school way?

luizfbicalho commented 3 months ago

A couple thoughts.

First, when using private fields, a class must implement the IMobileObject interface methods to set/get the state of those private fields so serialization works. The generator should generate those methods as well (while still honoring any manual implementation of those methods).

Isn't better to use the other generator for the AutoSerialization?

Second, the AutoProperty attribute needs the ability to handle all the different property scenarios:

  • public get/set
  • public get, private set
  • private property with get/set

I'll take a look in other options , but we could at first think of an enum here right?

Third, the AutoProperty attribute needs to work based on the base class (keeping in mind that poeple often add custom base types into the inheritance tree, so we need to find the base class anywhere in the tree):

  • BusinessBase - public or private properties, with public properties maybe having a private set
  • ReadOnlyBase - public read-only properties (so maybe a private set, usually with LoadProperty)
  • CommandBase - public properties, but always implemented with ReadProperty and LoadProperty

This case I think shoudn't be automatic, because you could add any option os property in this case

Fourth, another variation on the theme is that a string property can be backed by a SmartDate value. In this case, the field would be of type SmartDate, and the property would be of type string. However, it is also possible for someone to directly expose the SmartDate value as the property type - so both scenarios need to be available.

We can add an optional type here, or we can leave it outside the generator if it's only a very exceptional case

Fifth, the way you implement a non-serializable property is by using a private backing field and not including that field value in the get/set state methods of IMobileObject. The generator can probably key off the NonSerializable attribute if it is attached to the field, so we don't need a new attribute of our own.

Does this should work on the AutoSerialization? I think that they should work together to achieve this

It occurs to me that we also need a way to set (and localize) the FriendlyName of the property meta-state in the generated code.

I agree with @kant2002 on this, we should make it work for the 80% of the cases you have, and then after that work the way up.

StefanOssendorf commented 3 months ago

I'm with @kant2002 and @luizfbicalho. We should start with typical property implementations and improve it from there. Even the simple public get/set is a huge improvement and boilderplate reduction imho.

Maybe we can get some ideas from the MVVM part of this repo https://github.com/CommunityToolkit/dotnet?tab=readme-ov-file#-what-does-this-repo-contain . At work we are using this to auto generate the whole property boilerplate stuff.

rockfordlhotka commented 3 months ago

I agree with everyone that this should be an iterative process to build out the generator. I agree that version 0.1 should totally focus on the simple public get/set property scenario in BusinessBase.

At the same time, I think it is helpful to get a list of things we know need to exist eventually, so the API doesn't have to be completely discarded from v0.1 to 0.2 (for example).

A couple thoughts. First, when using private fields, a class must implement the IMobileObject interface methods to set/get the state of those private fields so serialization works. The generator should generate those methods as well (while still honoring any manual implementation of those methods).

Isn't better to use the other generator for the AutoSerialization?

I would prefer if we allow people to choose. I think it'll get more immediate use if it supports MobileFormatter out of the box. There's also the impressive work @JasonBock has been doing on a serialization generator, and of course the AutoSerialization generator.

If we support MobileFormatter out of the box, the other two should still be options without extra effort.

Second, the AutoProperty attribute needs the ability to handle all the different property scenarios:

  • public get/set
  • public get, private set
  • private property with get/set

I'll take a look in other options , but we could at first think of an enum here right?

Maybe a Flags enum.

Third, the AutoProperty attribute needs to work based on the base class (keeping in mind that poeple often add custom base types into the inheritance tree, so we need to find the base class anywhere in the tree):

  • BusinessBase - public or private properties, with public properties maybe having a private set
  • ReadOnlyBase - public read-only properties (so maybe a private set, usually with LoadProperty)
  • CommandBase - public properties, but always implemented with ReadProperty and LoadProperty

This case I think shoudn't be automatic, because you could add any option os property in this case

It should be somewhat automatic, or have an analyzer. Specifically because you can't use SetProperty in a ReadOnlyBase or CommandBase, and you can't use GetProperty in a CommandBase.

Though I suppose we can let the compiler tell the developer that they made a mistake, so it isn't like the build would work if they do it wrong.

Fourth, another variation on the theme is that a string property can be backed by a SmartDate value. In this case, the field would be of type SmartDate, and the property would be of type string. However, it is also possible for someone to directly expose the SmartDate value as the property type - so both scenarios need to be available.

We can add an optional type here, or we can leave it outside the generator if it's only a very exceptional case

This one is mainstream for people who do use SmartDate, but I don't know how widely used that type is anymore. I think you are right, this should be left out - for sure of the first iteration of the generator.

Fifth, the way you implement a non-serializable property is by using a private backing field and not including that field value in the get/set state methods of IMobileObject. The generator can probably key off the NonSerializable attribute if it is attached to the field, so we don't need a new attribute of our own.

Does this should work on the AutoSerialization? I think that they should work together to achieve this

I doubt that the existing auto serialization generator honors any sort of non-serialized concept. That's a feature that should be added, because it is important to support calculated properties that don't need to be serialized because they are always calculated by a rule.

It occurs to me that we also need a way to set (and localize) the FriendlyName of the property meta-state in the generated code.

I agree with @kant2002 on this, we should make it work for the 80% of the cases you have, and then after that work the way up.

I suspect that most non-US software (especially EU and Asia) use localization on a regular basis. I agree that this doesn't need to be in the first iteration, but supporting multiple language/culture settings is (imo) quite important for CSLA as a globally used framework.

rockfordlhotka commented 3 months ago

I was just thinking how much more clear this would be if we had partial properties, so we could attach the AutoSerialized attribute to a property instead of a field. It turns out this is part of .NET 9 and C# 13.

https://devblogs.microsoft.com/dotnet/csharp-13-explore-preview-features/

So, I propose we plan for the released version of this feature to work off a partial property, even though v0.1 will work off a field.

  [AutoSerialized]
  public partial string Name();

A key benefit of this, imo, is that it would allow us to generate a managed property instead of a private field property. That would eliminate the whole question of generating code to support any particular serializer (like MobileFormatter), because all CSLA serializers need to support managed properties.

Update

It occurs to me that even v0.1 could trigger off the field, but then generate a managed property - totally ignoring the field declaration. That way the generator can be created like we want it for final release. The only drawback is that while in testing the CSLA 9 prereleases will leave some unused fields in the code - which shouldn't bother anyone, because they aren't used for or by anything.

So this

  [AutoSerialized]
  private string _name;

would generate this:

    public static readonly PropertyInfo<string> NameProperty = RegisterProperty<string>(nameof(Name));
    public string Name
    {
        get => GetProperty(NameProperty);
        set => SetProperty(NameProperty, value);
    }
luizfbicalho commented 3 months ago

I was just thinking how much more clear this would be if we had partial properties, so we could attach the AutoSerialized attribute to a property instead of a field. It turns out this is part of .NET 9 and C# 13.

Nice, I think that we should work for this release

rockfordlhotka commented 3 months ago

I'm very excited about this!!

kant2002 commented 3 months ago

@rockfordlhotka I think you mixed up my message a bit. I did not say anything about localization of source gen.but while we are here. In my experience localization do more harm then good, except probably German and maybe Chinese. Localized messages conplegely not googleable. It novice ask for help then experienced developer completely dumbfounded since he use English as lingua-franca. Personally I’m for localization and even more of it but that’s current state.

anyway. My point that we should not overcompicate source gen because that quickly lead to have duplicate support surface and we will have second DSL in addition to C#. All for saving couple keystrokes. That’s does not feel right.

luizfbicalho commented 3 months ago

I'm very excited about this!!

me too, it's my first contribution to this project

rockfordlhotka commented 3 months ago

@rockfordlhotka I think you mixed up my message a bit. I did not say anything about localization of source gen.but while we are here. In my experience localization do more harm then good, except probably German and maybe Chinese. Localized messages conplegely not googleable. It novice ask for help then experienced developer completely dumbfounded since he use English as lingua-franca. Personally I’m for localization and even more of it but that’s current state.

anyway. My point that we should not overcompicate source gen because that quickly lead to have duplicate support surface and we will have second DSL in addition to C#. All for saving couple keystrokes. That’s does not feel right.

I hear you. The FriendlyName is displayed to the end user though, and so it is something that is often localized.

I think our latest thinking about using partial properties in C# 13 will solve the localization problem all by itself, which is nice.

The easiest way to localize FriendlyName today is to use either of the "displayname" attributes on the property, because they support localization, and CSLA uses them for FriendlyName if they are on the property.

As a result, without any work by the generator, I think this will work:

  [AutoSerialized]
  [DisplayName(....)]
  public partial string Name();

The developer can use the DisplayName attribute as normal, providing the resource key to find the localized value, and the existing CSLA implementation will use it like today.

StefanOssendorf commented 3 months ago

@rockfordlhotka I think you mixed up my message a bit. I did not say anything about localization of source gen.but while we are here. In my experience localization do more harm then good, except probably German and maybe Chinese. Localized messages conplegely not googleable. It novice ask for help then experienced developer completely dumbfounded since he use English as lingua-franca. Personally I’m for localization and even more of it but that’s current state.

anyway. My point that we should not overcompicate source gen because that quickly lead to have duplicate support surface and we will have second DSL in addition to C#. All for saving couple keystrokes. That’s does not feel right.

I'm with you. I think we should only localize strings which are facing end users and not developers. I'm programming purely in english even though my native language is german. You have so much more options when programming in english because all popular libraries are in english. You can't avoid english as a developer. And TBH. To find CSLA you need to know english otherwise you would pick it by random? I can't imagine that :D

luizfbicalho commented 3 months ago
  [AutoSerialized]
  [DisplayName(....)]
  public partial string Name();

More than that

we can't be sure about the syntax of the feature, it could change until the release but this could be a possility

   [AutoSerialized]
   [DisplayName(....)]
   public partial string Name {get; private set;}

this way we can control in the generator what is private, public without any extra code

StefanOssendorf commented 3 months ago

@rockfordlhotka I think you mixed up my message a bit. I did not say anything about localization of source gen.but while we are here. In my experience localization do more harm then good, except probably German and maybe Chinese. Localized messages conplegely not googleable. It novice ask for help then experienced developer completely dumbfounded since he use English as lingua-franca. Personally I’m for localization and even more of it but that’s current state. anyway. My point that we should not overcompicate source gen because that quickly lead to have duplicate support surface and we will have second DSL in addition to C#. All for saving couple keystrokes. That’s does not feel right.

I hear you. The FriendlyName is displayed to the end user though, and so it is something that is often localized.

I think our latest thinking about using partial properties in C# 13 will solve the localization problem all by itself, which is nice.

The easiest way to localize FriendlyName today is to use either of the "displayname" attributes on the property, because they support localization, and CSLA uses them for FriendlyName if they are on the property.

As a result, without any work by the generator, I think this will work:

  [AutoSerialized]
  [DisplayName(....)]
  public partial string Name();

The developer can use the DisplayName attribute as normal, providing the resource key to find the localized value, and the existing CSLA implementation will use it like today.

You can do that actually with a field, too - I think. You can do the following:

[AutoProperty]
[property: DisplayName(...)]
private string _name;

Edit: Here's a real world doc which uses that: https://learn.microsoft.com/en-us/dotnet/communitytoolkit/mvvm/generators/observableproperty#adding-custom-attributes

luizfbicalho commented 3 months ago

You can do that actually with a field, too - I think.

Why? I mean, is there any case where a field is really needed? It's just to understand more scenarios

StefanOssendorf commented 3 months ago

You can do that actually with a field, too - I think.

Why? I mean, is there any case where a field is really needed? It's just to understand more scenarios

That only supports scenarios where partial properties are not supported.

kant2002 commented 3 months ago

I think field is needed for users of .net 8 SDK. Because not everybody would like to switch LangVersion to Preview or custom value

luizfbicalho commented 3 months ago

That only supports scenarios where partial properties are not supported.

Now I'm not sure if we should create 2 generators, one for field and one for partial property, because they could eventually go in a very different path one from the other

StefanOssendorf commented 3 months ago

Why can't we support both along side each other?

luizfbicalho commented 3 months ago

Why can't we support both along side each other?

I'm thinking and writing here, but one point is that the Attribute should have a lot of parameters and should be confusing if a set of parameters work for a property and a set of parameters work for a field, would be simpler to have 2 attributes, each one with only the properties used in each generator

StefanOssendorf commented 3 months ago

I don't see why we need two generators. We can have one generator supporting two attributes. Or am I misunderstanding your meaning of "used in each generator"?

luizfbicalho commented 3 months ago

I don't see why we need two generators. We can have one generator supporting two attributes. Or am I misunderstanding your meaning of "used in each generator"?

when I say 2, it could be 90% shared code between them, but I think that they could evolve in different paths, if we look at just what we talked today, most of the code in the property generated wont exist in the partial property,

we could change this on the way, but seems to me that we will add features on the way that won't mix in both cases, I think that when I start this issue we could talk more with something on the plate, what do you think?

I don't want to break any bridge now, could be one, or two, but now I can't see the best approach

StefanOssendorf commented 3 months ago

I fully agree. We should work that out during development. If it ends with more than one source gen that's fine too :-)

luizfbicalho commented 3 months ago

I had a different Idea, I think that this idea makes the partial property and the legacy versions a bit closer, and a cleaner code also

[AutoImplementInterface<IBusiness1>]
public partial class Business:BusinessBase 
{
}

public interface IBusiness1
{
    [DisplayAttribute()]
    string Name {get;set;}
}

//generated
public partial class Business: IBusiness1
{
    public static readonly PropertyInfo<string> NameProperty = RegisterProperty<string>(nameof(Name));
    public string Name
    {
        get => GetProperty(NameProperty);
        set => SetProperty(NameProperty, value);
    }
}

the interface is required only on the legacy version, in the preview we can get all partial properties this way we can get the private set if the interface just have the get.

and we can bring all the attributes from the interface

rockfordlhotka commented 3 months ago

@luizfbicalho It seems like the end user has to create a lot of extra types that aren't intuitive, but why don't you give it a try and see if we like it?

I don't know about you, but I usually fly through a bunch of prototype implementations of a new idea (or API) to see what they "feel like" in real use.

luizfbicalho commented 3 months ago

I created some examples andI'll put here for you to give your thoughts.

 [AutoImplementProperties]
  public partial class ReadOnlyPOCO : ReadOnlyBase<ReadOnlyPOCO>
  {

    public partial string Name { get; private set; }

  }

with this generated code

//<auto-generated/>
#nullable enable
using System;
using Csla;

namespace Csla.Generator.AutoImplementProperties.CSharp.TestObjects;

[Serializable]
public partial class ReadOnlyPOCO
{
    public static readonly PropertyInfo<string> NameProperty = RegisterProperty<string>(nameof(Name));
    public partial string Name
    {
        get => GetProperty(NameProperty);
        private set => LoadProperty(NameProperty, value);
    }

}

this is a command base

using Csla.Serialization;
namespace Csla.Generator.AutoImplementProperties.TestObjects
{
  [AutoImplementProperties]
  public partial class CommandPOCO : CommandBase<CommandPOCO>
  {
    public partial string CommandName { get; set; }
    public partial string? CommandDescription { get; set; }
    public partial string? CommandCode { get; set; }
  }
}

with this generated code

//<auto-generated/>
#nullable enable
using System;
using Csla;

namespace Csla.Generator.AutoImplementProperties.TestObjects;

[Serializable]
public partial class CommandPOCO
{
    public static readonly PropertyInfo<string> CommandNameProperty = RegisterProperty<string>(nameof(CommandName));
    public partial string CommandName
    {
        get => ReadProperty(CommandNameProperty);
        set => LoadProperty(CommandNameProperty, value);
    }
    public static readonly PropertyInfo<string?> CommandDescriptionProperty = RegisterProperty<string?>(nameof(CommandDescription));
    public partial string? CommandDescription
    {
        get => ReadProperty(CommandDescriptionProperty);
        set => LoadProperty(CommandDescriptionProperty, value);
    }
    public static readonly PropertyInfo<string?> CommandCodeProperty = RegisterProperty<string?>(nameof(CommandCode));
    public partial string? CommandCode
    {
        get => ReadProperty(CommandCodeProperty);
        set => LoadProperty(CommandCodeProperty, value);
    }

}

this third code

using System.ComponentModel.DataAnnotations;
using Csla.Serialization;

namespace Csla.Generator.AutoImplementProperties.CSharp.TestObjects
{
  [AutoImplementProperties]
  public partial class AddressPOCO : BusinessBase<AddressPOCO>
  {
    [Display(Name = "Address Line 1")]
    public partial string? AddressLine1 { get; private set; }

    public partial string AddressLine2 { get; set; }

    public partial string Town { get; set; }

    public partial string County { get; set; }

    public partial string Postcode { get; set; }

  }
}

with this generated code


//<auto-generated/>
#nullable enable
using System;
using Csla;

namespace Csla.Generator.AutoImplementProperties.CSharp.TestObjects;

[Serializable]
public partial class AddressPOCO
{
    public static readonly PropertyInfo<string?> AddressLine1Property = RegisterProperty<string?>(nameof(AddressLine1));
    public partial string? AddressLine1
    {
        get => GetProperty(AddressLine1Property);
        private set => SetProperty(AddressLine1Property, value);
    }
    public static readonly PropertyInfo<string> AddressLine2Property = RegisterProperty<string>(nameof(AddressLine2));
    public partial string AddressLine2
    {
        get => GetProperty(AddressLine2Property);
        set => SetProperty(AddressLine2Property, value);
    }
    public static readonly PropertyInfo<string> TownProperty = RegisterProperty<string>(nameof(Town));
    public partial string Town
    {
        get => GetProperty(TownProperty);
        set => SetProperty(TownProperty, value);
    }
    public static readonly PropertyInfo<string> CountyProperty = RegisterProperty<string>(nameof(County));
    public partial string County
    {
        get => GetProperty(CountyProperty);
        set => SetProperty(CountyProperty, value);
    }
    public static readonly PropertyInfo<string> PostcodeProperty = RegisterProperty<string>(nameof(Postcode));
    public partial string Postcode
    {
        get => GetProperty(PostcodeProperty);
        set => SetProperty(PostcodeProperty, value);
    }

}

and the last one is for the .NET without partial properties, as you can see I use an interface just to mark the code, like a region

using System.ComponentModel.DataAnnotations;
using Csla.Serialization;

namespace Csla.Generator.AutoImplementProperties.TestObjects
{
  [AutoImplementPropertiesInterface<IInterfaceImplementBusinessPOCO>]
  public partial class InterfaceImplementBusinessPOCO : BusinessBase<InterfaceImplementBusinessPOCO>
  {
    private interface IInterfaceImplementBusinessPOCO
    {
      int Id { get; }
      [Display(Name = "Interface Name")]
      string Name { get; set; }
      string? Description { get; set; }
      string? Code { get; }
    }
  }
}

and this is the generated code

//<auto-generated/>
#nullable enable
using System;
using Csla;

namespace Csla.Generator.AutoImplementProperties.TestObjects;

[Serializable]
public partial class InterfaceImplementBusinessPOCO
{
    public static readonly PropertyInfo<int> IdProperty = RegisterProperty<int>(nameof(Id));
    public int Id
    {
        get => GetProperty(IdProperty);
        private set => SetProperty(IdProperty, value);
    }
    public static readonly PropertyInfo<string> NameProperty = RegisterProperty<string>(nameof(Name));
    [System.ComponentModel.DataAnnotations.Display( Name="Interface Name")]
    public string Name
    {
        get => GetProperty(NameProperty);
        set => SetProperty(NameProperty, value);
    }
    public static readonly PropertyInfo<string?> DescriptionProperty = RegisterProperty<string?>(nameof(Description));
    public string? Description
    {
        get => GetProperty(DescriptionProperty);
        set => SetProperty(DescriptionProperty, value);
    }
    public static readonly PropertyInfo<string?> CodeProperty = RegisterProperty<string?>(nameof(Code));
    public string? Code
    {
        get => GetProperty(CodeProperty);
        private set => SetProperty(CodeProperty, value);
    }

}

the interface has the advantage that I can flow all the attributes from the interface to the class

rockfordlhotka commented 3 months ago

I was thinking about an attribute per-property rather than per-class, but reading through your examples I like how clean it is for any partial property to be automatically elevated to a managed property.

If I declare a property that isn't partial, then it doesn't generate code? So it would still be possible to have non-CSLA properties in my class - which might be a good thing.

The interface idea in the last example is clever. I wouldn't have thought of that (and didn't know you could actually do that - using the private interface in the BusinessBase<IPrivateInterface> - in fact, that really can't work can it? Won't the compiler get upset?

luizfbicalho commented 3 months ago

I was thinking about an attribute per-property rather than per-class, but reading through your examples I like how clean it is for any partial property to be automatically elevated to a managed property.

If I declare a property that isn't partial, then it doesn't generate code? So it would still be possible to have non-CSLA properties in my class - which might be a good thing.

Only the partial properties are implemented

The interface idea in the last example is clever. I wouldn't have thought of that (and didn't know you could actually do that - using the private interface in the BusinessBase<IPrivateInterface> - in fact, that really can't work can it? Won't the compiler get upset?

I'm not implementing the interface, you can see that I'm just copying the properties from the interface to the class, so it doesn't matter how the interface is, it's probably ignored elsewhere

rockfordlhotka commented 3 months ago

I like what you are proposing.

I'd like to hear from @StefanOssendorf and @TheCakeMonster too if they have time.

StefanOssendorf commented 3 months ago

My cents in no particular order:

Anyways I want such a generator to be created in csla because it can reduce the boilerplate code by a lot.

luizfbicalho commented 3 months ago

My cents in no particular order:

  • If we have a class wide attribute we should provide an "anti-attribute" to exclude properties from being targets of the generation. For example I'm using the MVVM Community Toolkit at work which generates a property with the notify property change boilerplate. I assume they will also support partial properties when available. So with that approach that would be impossible to use besides each other.

It's a good idea

  • We don't need the [Serializable] attribute any more. The requirement is dropped with v9. It's already removed, I removed just after my comment
  • The Attributename could be CslaPropertiesAttribute (class wide) and CslaPropertyAttribute (field?) and CslaImplentedAttribute (property) for "better" naming? AutoImplementingProperties is rather generic and could be anything imho.

I'm open to a pool for a better option, what name do you all like most?

  • It's probably for clarity but the generation should use globally qualified type names. For example global::Csla.PropertyInfo<int> to avoid ambigous type issues.

The other generator wasn't using this and I just kept them close, we could refactor them with some more best practices after the V1

  • The interface is smart. I would've not thought of that :D. On the other hand it kind of looks abused to get the attributes working, right? Couldn't be a field used with [property: DisplayName("MyName")] which can be carried over to the generated property?

My main idea was to just declare the property, and move away from the field option, that we would have a lot more code to control

Anyways I want such a generator to be created in csla because it can reduce the boilerplate code by a lot.

I want to see it in action so we can fell what else is needed

StefanOssendorf commented 3 months ago
  • The interface is smart. I would've not thought of that :D. On the other hand it kind of looks abused to get the attributes working, right? Couldn't be a field used with [property: DisplayName("MyName")] which can be carried over to the generated property?

My main idea was to just declare the property, and move away from the field option, that we would have a lot more code to control

From a generator author perspective I agree with you. Less work. But from a user perspective it looks awkward to have a private nested interface to define properties which are potentially not even only implemented in the enclosing type. Is this possible?

[AutoImplementPropertiesInterface<IInterfaceImplementBusinessPOCO>]
public partial class InterfaceImplementBusinessPOCO : BusinessBase<InterfaceImplementBusinessPOCO>
{
  private interface IInterfaceImplementBusinessPOCO
  {
    int Id { get; }
    [Display(Name = "Interface Name")]
    string Name { get; set; }
    string? Description { get; set; }
    string? Code { get; }
  }
}

[AutoImplementPropertiesInterface<IInterfaceImplementBusinessPOCO>]
public partial class TotallyOtherBusinessPOCO : BusinessBase<TotallyOtherBusinessPOCO>
{
}
luizfbicalho commented 3 months ago

But from a user perspective it looks awkward to have a private nested interface to define properties which are potentially not even only implemented in the enclosing type. Is this possible?

I put it as a private interface for the user to see the code where it will be generated, but you are free to put the interface in a another file, from the user perspective I saved many files and just added 3 lines of code, and it's just until .net 9, I think this is the simplest solution to add properties

It's possible to add this interface to many files.

rockfordlhotka commented 3 months ago

My cents in no particular order:

  • If we have a class wide attribute we should provide an "anti-attribute" to exclude properties from being targets of the generation. For example I'm using the MVVM Community Toolkit at work which generates a property with the notify property change boilerplate. I assume they will also support partial properties when available. So with that approach that would be impossible to use besides each other.

It's a good idea

If we're going to do per-property attributes, I'd prefer an opt-in model than an opt-out model. In other words, drop the class level attribute, and only have a per-property attribute to include the property for generation. Like CslaPropertyAttribute.

  • We don't need the [Serializable] attribute any more. The requirement is dropped with v9. It's already removed, I removed just after my comment
  • The Attributename could be CslaPropertiesAttribute (class wide) and CslaPropertyAttribute (field?) and CslaImplentedAttribute (property) for "better" naming? AutoImplementingProperties is rather generic and could be anything imho.

I'm open to a pool for a better option, what name do you all like most?

If we go with per-property instead of per-class, I prefer CslaProperty, and perhaps CslaField if we decide to support private backing fields. Personally, for v1 anyway, I think we should continue to focus on managed properties and ignore private backing fields.

  • It's probably for clarity but the generation should use globally qualified type names. For example global::Csla.PropertyInfo<int> to avoid ambigous type issues.

The other generator wasn't using this and I just kept them close, we could refactor them with some more best practices after the V1

Good call. For all generators.

  • The interface is smart. I would've not thought of that :D. On the other hand it kind of looks abused to get the attributes working, right? Couldn't be a field used with [property: DisplayName("MyName")] which can be carried over to the generated property?

My main idea was to just declare the property, and move away from the field option, that we would have a lot more code to control

Anyways I want such a generator to be created in csla because it can reduce the boilerplate code by a lot.

I want to see it in action so we can fell what else is needed

It feels like we're getting close to something worth seeing in action. I think we need to resolve the per-property vs per-type attribute question.

I do lean toward the per-property attribute, as I think it best to have that level of control, even though it does add more code.

luizfbicalho commented 3 months ago

I do lean toward the per-property attribute, as I think it best to have that level of control, even though it does add more code.

I think the other way, the option per class I think that is easier to start, then after you get used to it, maybe we will need some fine tuning, but if we start by property or field, it's harder for the user to start using.

You are the formula 1 driver, you need all the customizations that you can get, but 80% need to get the car running first

rockfordlhotka commented 3 months ago

I do lean toward the per-property attribute, as I think it best to have that level of control, even though it does add more code.

I think the other way, the option per class I think that is easier to start, then after you get used to it, maybe we will need some fine tuning, but if we start by property or field, it's harder for the user to start using.

You are the formula 1 driver, you need all the customizations that you can get, but 80% need to get the car running first

I am laughing, because I remember the WCF team debating this in the early days of designing the whole DataContract concept for the DataContractSerializer. Opt-in vs opt-out - explicit code vs convenience. They ultimately chose the opt-in approach.

It is also convention vs configuration, and similar debates the industry has had over the years.

I can be talked into the opt-out approach as long as we build that in from the start with some sort of attribute. I do think there's an argument to be made there, in that most properties will be CSLA properties most of the time, so following the model that Microsoft did with Serializable and NotSerialized seems pretty valid.

So maybe we have ImplementCslaProperties and NotCslaProperty as our attributes?

Or CslaImplementProperties and CslaIgnoreProperty?

rockfordlhotka commented 3 months ago

You are the formula 1 driver, you need all the customizations that you can get, but 80% need to get the car running first

My one comment on this, is that we can do anything we want between now and November. But after CSLA 9 releases, changing the generator, especially in any big way, becomes much harder because it would break people's code.

Now's the time to experiment for sure!

luizfbicalho commented 3 months ago

You are the formula 1 driver, you need all the customizations that you can get, but 80% need to get the car running first

My one comment on this, is that we can do anything we want between now and November. But after CSLA 9 releases, changing the generator, especially in any big way, becomes much harder because it would break people's code.

Now's the time to experiment for sure! I can be talked into the opt-out approach as long as we build that in from the start with some sort of attribute. I do think there's an argument to be made there, in that most properties will be CSLA properties most of the time, so following the model that Microsoft did with Serializable and NotSerialized seems pretty valid.

So maybe we have ImplementCslaProperties and NotCslaProperty as our attributes?

Or CslaImplementProperties and CslaIgnoreProperty? I think that implement and ignore are better for clarity

We could add this almost finished one, with the option to ignore some other partial property that could be useful for MVVM and rename them to be more meaningfull, this is our V1

We could add the global and I have another point to the previous generator, this is V1.1

after that we can try a new generator that is more refined, to work in a property, then we can see with a little usage experience what we missed out of the first version and we use the complains to improve to V2

luizfbicalho commented 3 months ago

I changed the names of the attributes, added the ignore feature

[CslaImplementProperties]
public partial class AddressPOCO : BusinessBase<AddressPOCO>
{
  [Display(Name = "Address Line 1")]
  public partial string? AddressLine1 { get; private set; }
  public partial string AddressLine2 { get; set; }
  public partial string Town { get; set; }
  public partial string County { get; set; }
  public partial string Postcode { get; set; }
  [CslaIgnoreProperty]
  public partial string IgnoredProperty { get; set; }
}

public partial class AddressPOCO
{
  public partial string IgnoredProperty { get => ""; set { } }
}

//<auto-generated/>
#nullable enable
using System;
using Csla;

namespace Csla.Generator.AutoImplementProperties.CSharp.TestObjects;

public partial class AddressPOCO
{
    public static readonly PropertyInfo<string?> AddressLine1Property = RegisterProperty<string?>(nameof(AddressLine1));
    public partial string? AddressLine1
    {
        get => GetProperty(AddressLine1Property);
        private set => SetProperty(AddressLine1Property, value);
    }
    public static readonly PropertyInfo<string> AddressLine2Property = RegisterProperty<string>(nameof(AddressLine2));
    public partial string AddressLine2
    {
        get => GetProperty(AddressLine2Property);
        set => SetProperty(AddressLine2Property, value);
    }
    public static readonly PropertyInfo<string> TownProperty = RegisterProperty<string>(nameof(Town));
    public partial string Town
    {
        get => GetProperty(TownProperty);
        set => SetProperty(TownProperty, value);
    }
    public static readonly PropertyInfo<string> CountyProperty = RegisterProperty<string>(nameof(County));
    public partial string County
    {
        get => GetProperty(CountyProperty);
        set => SetProperty(CountyProperty, value);
    }
    public static readonly PropertyInfo<string> PostcodeProperty = RegisterProperty<string>(nameof(Postcode));
    public partial string Postcode
    {
        get => GetProperty(PostcodeProperty);
        set => SetProperty(PostcodeProperty, value);
    }
}
rockfordlhotka commented 3 months ago

I like the look of that approach.

luizfbicalho commented 3 months ago

The unit tests aren't working, I created an issue in the roslyn test component. Probably I did something wrong and can't see what is that

https://github.com/dotnet/roslyn-sdk/issues/1173

StefanOssendorf commented 3 months ago

I'm using "verify" to verify the sourc at the end. But maybe this can help? The whole magic happens here in my unit tests: https://github.com/StefanOssendorf/Csla.DataPortalExtensions/blob/master/tests/Csla.DataPortalExtensionGenerator.Tests/Helper/SourceGenTester.cs

luizfbicalho commented 3 months ago

I'm using "verify" to verify the sourc at the end. But maybe this can help? The whole magic happens here in my unit tests: https://github.com/StefanOssendorf/Csla.DataPortalExtensions/blob/master/tests/Csla.DataPortalExtensionGenerator.Tests/Helper/SourceGenTester.cs

Thanks for the help, my problem was the missing references for the compilation, I'll try to make your helpers generic to use without much change