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

WrapperValueObject source generator #17

Closed
YairHalberstadt opened this issue Oct 20, 2020 · 12 comments · Fixed by #18
Closed

WrapperValueObject source generator #17

YairHalberstadt opened this issue Oct 20, 2020 · 12 comments · Fixed by #18

Comments

@YairHalberstadt
Copy link
Contributor

Saw this - thought it looked cool and you might be interested https://github.com/martinothamar/WrapperValueObject

@ironcev
Copy link
Owner

ironcev commented Oct 20, 2020

Thanks @YairHalberstadt! Very simple but definitely cool. It demonstrates an additional area in which source generators can be used, for generating types with a predefined structure. A kind of a C# 9.0 record feature for structs in this case ;-)

It reminds me also of @andrewlock's excellent article series Using strongly-typed entity IDs to avoid primitive obsession and his StronglyTypeId project.

WrapperValueObject is still in a very early I would say PoC stage but demonstrate well an additional area of source generators usage so I would like to add it to the Awesome Roslyn list. @YairHalberstadt would you mind contributing and creating a pull request?

@andrewlock
Copy link

Excellent! I tried out a simple PoC of this a while ago, that looked almost exactly like this 🙂 Couldn't be more timely, seeing as the build for my StrongTypedId project seems to have imploded in an SDK update at some point 😫

@ironcev
Copy link
Owner

ironcev commented Oct 20, 2020

@andrewlock although you've mentioned in the last article in the series that you do not see a value in porting StronglyTypedId to source generators, I would love to see that happening. The existing implementation is great and also the whole series of blog posts is the best read on the topic I came across. Having a C# 9.0 source generator of that quality would be great.

On the other hand, I would equally love to see @martinothamar's WrapperValueObject moving beyond the PoC, getting maybe cleaner API, Nuget package etc. At the moment it is a combination of a strongly typed id and an immutable struct generator.

Anyhow, thank you both for your inspiring work! :-)

ironcev pushed a commit that referenced this issue Oct 21, 2020
WrapperValueObject proof-of-concept demonstrates very well how source generators can be used for generating types of a predefined structure like immutable structs, strongly typed ids and similar. 

Fixes #17.
@martinothamar
Copy link

Hey! Cool, thanks for the mention

A kind of a C# 9.0 record feature for structs in this case ;-)

My original motivation was simplifying lots of boilerplate related to value object structs in a previous project, but I also ended up at this exact conclusion - it's basically record feature for structs 🙂 C# 9 only comes with the record feature for classes thus far AFAIK, is that still correct? Do you know if there has been any consideration for doing it also for structs in the future? The .NET guidance for defining structs is to always implement GetHashCode and Equals AFAIK since if equality/hashcode is used in any way without those implementations it will have massive performance implications (experienced this in production once)

WrapperValueObject is still in a very early I would say PoC stage

Indeed it is, haven't used this much yet. But since it got some interest I'll add some polish and get a proper nuget package out. Suggestions etc are welcome 👍

@YairHalberstadt
Copy link
Contributor Author

@martinothamar

For me my main usage would be creating strongly typed wrappers for primitives - eg. MetersLength which wraps int, and can be implicitly cast between the two.

I think you already generate all that, but some documentation on what's generated would be useful!

@YairHalberstadt
Copy link
Contributor Author

Do you know if there has been any consideration for doing it also for structs in the future?

That's planned for C# 10

@martinothamar
Copy link

That was my usecase as well, awesome, will put in some docs work and publish 👍

@andrewlock
Copy link

I'd be keen to retire my StronglyTypedId package in favour of your source generator version too - my intention was to rewrite StronglyTypedId using source gens once it went GA, but looks like you've saved me the trouble 😀 There may be space for both of them, serving slightly different goals, but we'll see - if I can get away using yours that sounds good to me 🙂

@ironcev
Copy link
Owner

ironcev commented Oct 21, 2020

My suggestion (I hope I am not shooting too far :-)) is to bring WrapperValueObject to a production level and to have the strongly typed ids as one of the supported use cases:

  • Improve API:
    • Replace one generic attribute (WrapperValueObject) with two (or more) that cleary identify the usecase. E.g. StronglyTypedIdAttribute, ImmutableStructAttribute, ...
  • Support everything that StronglyTypedId supports (e.g. optional generation of JSON converters).
  • Bring the documentation to the same level as in the StronglyTypedId project.
  • Write tests.
  • Create Nuget package.

@martinothamar
Copy link

@ironcev that sounds good to me, I can try to get there and maybe @andrewlock can review eventually, then we can see how it works out. I'm open to any solution, and agree with the usecases here 🙂

@martinothamar
Copy link

Hi again, I have pushed some updates

  • Improved README
  • NuGet package
  • More tests (still more needed)
  • Support more of StronglyTypedId functionality (JsonConverter is the remaining piece I think)

Other than that not quite sure on how to improve the API/Attribute(s). Been thinking of maybe having 1 attribute per primitive type for example - IntWrapperType, FloatWrapperType etc. Some of the code that is being generated should probably also be configurable, for instance maybe you don't want implicit casts. But I think this is an OK start at least.

@ironcev
Copy link
Owner

ironcev commented Oct 23, 2020

Looks good, thanks for your commitment @martinothamar :-) Regarding the API improvements and other ideas, I propose to move the discussion to the WrapperValueObject repository and use dedicated GitHub issues for that. I am interested in seeing the project getting developed further and can formulate the proposals more precisely in GitHub issues. That way it will be much easier to discuss and refine them one by one.

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

Successfully merging a pull request may close this issue.

4 participants