SteveDunn / Vogen

A semi-opinionated library which is a source generator and a code analyser. It Source generates Value Objects
Apache License 2.0
782 stars 46 forks source link

Implement IParsable<> #527

Closed CheloXL closed 4 months ago

CheloXL commented 10 months ago

Describe the feature

In .net 8 you have IParsable<> that is being used by the framework in several places. It should be nice if that interface could be also implemented as part of the generated code (maybe as an option in the configuration?).

I tried to implement it on one of my VOs, but the problem is that on the TryParse method, I don't have a way to return a default value without the entity throwing an exception (need to create an empty instance via attributes and have to return that, but that feels like a dirty hack).

SteveDunn commented 10 months ago

Thanks - an interesting idea. I'll take a look into that. Did you know that there is some support for parsing in Vogen; if the underlying type has a TryParse method, then Vogen will 'hoist' that up.

There's some tests here.

CheloXL commented 10 months ago

Yes. In fact, I found those while implementing IParsable manually by myself. The TryParse is already implemented, only missing the Parse method... but on non-string VO. On string VOs, of course there is no TryParse, so I had to implement both, and that's where my problem is: I have no way to "create" a default value for the false return option.

SteveDunn commented 9 months ago

Understood. I'll take a look when time permits, hopefully soon.

Thanks again for the feedback

CheloXL commented 7 months ago

This is tangential to this issue, but it could solve my problem (letting me implement TryParse), but may be useful so you don't have to implement X feature every time the frameworks adds something: Could it be possible to let me use default or new() inside the VO? Not sure if that's possible (don't know how the code analysis works for source generators). But in my case, I would be able to implement TryParse by myself and you would not have to implement any new "feature" for the VOs.

arteny commented 7 months ago

Yes, waiting for this feature, it is correlates with my feature request issue about asp.net

SteveDunn commented 6 months ago

I'm taking a look at this now. As we know, Vogen already has TryParse, but only where the wrapper pimitive type is a value type. I can't remember exactly why this limitation was made, but I'm now wondering if it makes sense. I'm wondering this because the Value Object's .Value throws an exception regardless of whether the wrapper primitive is a value type or not.

The current implementation for TryParse looks at the underlying type for public static methods named TryParse that return a bool and who's last parameter is the same type as the wrapped primitive.

I'm thinking that for this Issue, I just need to extend TryParse to also include Parse, and to include this whether the Value Object is a reference type or value type.

Adding Parse to a value object who's wrapped primitive is an int (e.g. [ValueObject<int>] public partial struct Score {}) requires these methods to be generated:

image

I don't yet know if this is a problem or not, but it seems excessive.

The source generator now generates all public static methods name Parse and delegates to the wrapped primitive implementation. This might be a breaking change to users who have implemented their own methods.

dgg commented 6 months ago

Roughly related to this issue (which I am solving myself with the delegation of all .Parse() overloads to the underlying type) comes the fact that the already generated .TryParse() do not really follow the .Tryxx() pattern of not throwing when custom validation is implemented.

In my case, the generated .TryParse() will throw a validation exception instead of returning false and not throwing, which I think should be the pattern.

Shouldn't be a way to disable this behavior of implementing non-normative .TryGet() methods? Shall I open a new issue for this matter or shall we "group" it with this?

dgg commented 6 months ago

I just found a new inconsistency with normative .TryParse() on structs (in my case a record struct) and that has to do with value objects relationship with default instances (or lack of). When invoking a failing normative .TryParse(), false is returned and the out parameter is set to default. Accessing that out parameter does not yield any side-effect. When doing the same on a value object, the access of the out parameter throws a ValueObjectValidationException : Use of uninitialized Value Object.

It may be a minor issue since one should not access a non-successfully parsed object, but it exemplifies the difference in behavior from generated .TryParse() methods and, in my opinion, the need to control the generation of such methods.

SteveDunn commented 5 months ago

Thanks for the feedback @dgg . I'm finishing up this bit of work now and will be releasing it soon. I would say that the exception when using an unitialized value via TryParse is correct. I'll add some more comments and notes on this thread soon. Thanks again

SteveDunn commented 5 months ago

Almost there. By default, Parse and TryParse methods are hoisted up from the underlying primitive.

image

Also, if the underlying primitive is a string, then IParsable<TSelf> is generated

image

image

CheloXL commented 5 months ago

One thing that it's not clear for me: Does TryParse calls the Validate method? I'm asking so I don't have to implement multiple validations (one in the Validate method, another in the TryParse). It would be nice if the TryParse would call the Validate method (if there is one) for validation, and the Parse method would throw the error returned by the Validate method, to remain consistent in every place.

SteveDunn commented 5 months ago

@CheloXL - TryParse does call a Validate method if there is one. However, beginning with the next version, it doesn't throw an exception; the out parameter is set to null for reference types, and default for value types (where the .Value throws an exception saying that the value object hasn't been initialized)

CheloXL commented 5 months ago

@SteveDunn That's perfect. Thanks!

SteveDunn commented 4 months ago

This is implemented in v4.0.0 which should be available later today.