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

Support also get-only properties #14

Closed
meirkr opened this issue Jan 27, 2022 · 7 comments
Closed

Support also get-only properties #14

meirkr opened this issue Jan 27, 2022 · 7 comments

Comments

@meirkr
Copy link

meirkr commented Jan 27, 2022

Hi
I like this project a lot.

It is possible to apply the behavior also in case of get-only properties (which has a backup field behind) ?
See this example, which expected to inject also the NonImplementedProp to the ctor.

    [AutoConstructor]
    public partial class SomeClass
    {
        private readonly int _someInt;
        private readonly string _someString;
        public int NonImplementedProp { get; } // hopefully to be auto injected as well

        public int SomeImplementedProp => 2;


        // TODO - auto generate also the property
        public SomeClass(int someInt, string someString, int nonImplementedProp)
        {
            _someInt = someInt;
            _someString = someString;
            NonImplementedProp = nonImplementedProp; // hopefully to be auto injected as well
        }
    }
@k94ll13nn3
Copy link
Owner

Hi,

I've released version 2.3.0 with support for injecting get-only properties.

I was a little against allowing this case at first, and I am still unsure about it. But I've decided to allow it in an opt-in manner because:

  • It would have been a breaking change otherwise and I didn't wanted to go to a 3.0 for now
  • The way I implemented it uses features of C# that have some drawbacks (like for example one diagnostic will not work)

So the injection is done by using the AutoInjectAttribute (in all the forms possibles) on the property with the field keyword:

[field: AutoConstructorInject]
public int Property { get; }

Using your sample:

[AutoConstructor]
public partial class SomeClass
{
    private readonly int _someInt;
    private readonly string _someString;

    [field: AutoConstructorInject]
    public int NonImplementedProp { get; }

    public int SomeImplementedProp => 2;

    public SomeClass(int someInt, string someString, int nonImplementedProp)
    {
        this._someInt = someInt;
        this._someString = someString;
        this.NonImplementedProp = nonImplementedProp;
    }
}

More infos in the README.

@k94ll13nn3 k94ll13nn3 added this to the v2.3.0 milestone Jan 27, 2022
@meirkr
Copy link
Author

meirkr commented Jan 29, 2022

Hi
First, great to know there is such capability based on the backing field attribute. Tx.

Second,
About:

It would have been a breaking change otherwise and I didn't wanted to go to a 3.0 for now

What about the ability to prevent that "break" by adding to the AutoConstructorAttribute a propoerty like "bool InjectGetOnlyProperties"

    [AutoConstructor(InjectGetOnlyProperties=true)]
    public partial class SomeClass
    {
        private readonly int _someInt;
        private readonly string _someString;

        public int NonImplementedProp { get; } // hopefully to be auto injected as well

        public int SomeImplementedProp => 2;
    }

Only then, the generator will refer such properties and inject them as well.
No breaking change by that. Just another feature being added.

What do you think about this proposal?

@k94ll13nn3
Copy link
Owner

I'm sorry but I will maintain my position on this. I think it's ok for now if you need some get only properties injected, but if you need a lot, then it will be out the scope of what I wanted initially for this library.

I want to have a visual indicator on get only properties, and modifying the base attribute is not enough for me.

The breaking change issue was not a major point in my decision, just a additional one, and I think that even if I were to go to a 3.0, I would still not do the injection by default.

Hope it is ok for you.

@meirkr
Copy link
Author

meirkr commented Jan 31, 2022

Actually, the motivation is more about DTOs, which have only properties.
Now that C# has the Nullable checks feature, it would be a huge advantage implementing non null properties by constructor automatically.

BTW
That is true to settable properties as well.
The static code analysis complaints about null property and proposes to initialize it by a constructor (and of course - having an empty string initializing is not always a good solution).

Your mechanism brings a fantastic solution to implement it.

public class Person
{
    public string Name { get; set; }
    public string LastName { get; set; }
    public DateOnly BirthDate { get; set; }

    public Person(string name, string lastName, DateOnly birthDate)
    {
        Name = name;
        LastName = lastName;
        BirthDate = birthDate;
    }
}

Or, in an immutable DTO (records usage is not always efficient):

public class Person
{
    public string Name { get; }
    public string LastName { get; }
    public DateOnly BirthDate { get; }

    public Person(string name, string lastName, DateOnly birthDate)
    {
        Name = name;
        LastName = lastName;
        BirthDate = birthDate;
    }
}

Hmmm...
Maybe a different attribute could be proposed?
e.g [AutoPropertiesConstructor]
?

@k94ll13nn3
Copy link
Owner

I hadn't see it this way but your point is valid, and I agree that it could be a good feature for this case. I will think about how to do it but I will certainly do it.

@k94ll13nn3 k94ll13nn3 reopened this Jan 31, 2022
@meirkr
Copy link
Author

meirkr commented Feb 8, 2022

I found a similar project which already includes this capability.
You might like to check this...
https://github.com/chaowlert/PrimaryConstructor

@k94ll13nn3 k94ll13nn3 removed this from the v2.3.0 milestone Feb 9, 2022
@k94ll13nn3
Copy link
Owner

I have released version 3.0.0-beta.1 with support for get-only properties enabled by default (without having to use the attribute).

I have some more work to do before releasing the 3.0.0 version but this should be as stable as a non pre-release version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants