andrewlock / StronglyTypedId

A Rosyln-powered generator for strongly-typed IDs
MIT License
1.52k stars 79 forks source link

[Proposal] Add the ability to optionally provide an implicit conversion operator between the wrapper type and the primitive type #39

Closed SeanFarrow closed 10 months ago

SeanFarrow commented 3 years ago

It would be really nice if there was the ability to optionally add an implicit operator to convert the wrapped type to the primitive one.

This would negate the need to manually use the created type converter or access the value or ToString members directly.

andrewlock commented 2 years ago

Personally, I'm quite strongly against this one, it removes one of the big safety guards that strongly typed ids bring.. That said, having it as an option doesn't seem too harmful..

patrolez commented 2 years ago

Although, this kind of request looks like some kind of temptation and taking shortcuts which could turn growing/developing projects code to start "rotting" as it is being said in the Clean Code book.

Frameworks and abstracts came into life to avoid human errors and to not repeat things.

Personally, I would say that I would not make this kind of feature an official part of the code, but maybe give a space/placeholder to implement it in external way (if possible, if not so just I would do not provide any way to allow for such implicites), just to get away from claims/blames that this kind of project has built-in disadvantages.

But if such implicites would make it into this project (even as optional feature), so I think it should be done in a way that it would request a greatly visible footprint and amount of effort/energy to a developer who would introduce this drawback into someone's projects (so you would clearly know who to fire 🙃, or send for some training).

People who care about the code also needs to spend their holidays peacefully, so no one should introduce a mess into their project of interests at the time. StronglyTypedId without implicites is making a job which allows to keep such peace of mind, at least in one aspect.

SeanFarrow commented 2 years ago

This sounds like a good compromise, if .net incremental source generators allow this.

From: Patryk @.> Sent: 25 November 2021 23:12 To: andrewlock/StronglyTypedId @.> Cc: Sean Farrow @.>; Author @.> Subject: Re: [andrewlock/StronglyTypedId] [Proposal] Add the ability to optionally provide an implicit conversion operator between the wrapper type and the primitive type (#39)

Although, this kind of request looks like some kind of temptation and taking shortcuts which could turn growing/developing projects code to start "rotting" as it is being said in the Clean Code book.

Frameworks and abstracts came into life to avoid human errors and to not repeat things. I would say that I would not make this kind of feature an official part of the code, but maybe give a space/placeholder to implement it in external way, just to get away from claims that this kind of project has built-in disadvantages.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/andrewlock/StronglyTypedId/issues/39#issuecomment-979509934, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AALDK7VDUEQPOAWIV2XCCKTUN27D3ANCNFSM5DLIL54A.

BOBJohnson commented 2 years ago

https://github.com/BOBJohnson/StronglyTypedId/commit/53339c06cdc59e36f155892cb300ed143499a871 (original commit) https://github.com/BOBJohnson/StronglyTypedId/commit/7e4eb19ab8d14f708e25cb6229e8d2e1035fcb28 (revised commit)

If you get a chance, please review my fork + commit in reference to this proposal.

One thing I haven't done is updated any of the tests as I felt that was outside my wheelhouse with the limited understanding I have of your project.

I also didn't update the readme.

Considering that I haven't updated the tests (I did manual testing though with a test project and local version of package), nor updated the readme, I didn't feel it was ready for a PR just yet.


Original commit added new values to the StronglyTypedIdImplementations flags enum but I didn't like the fact you could specify mutually exclusively flags with how I had done it originally.

Revised commit split it out into a new StronglyTypedIdConversionOperator enum that had 3 values (None, Explicit, and Implicit). You can then specify the type of conversion operator from the base type and a conversion operator to the base type.

IE, you can now do: [StronglyTypedId(fromOperator: StronglyTypedIdConversionOperator.Explicit)] public partial struct MyId {}

This now allows the following code: MyId myId = (MyId)someGuidVariable;

SeanFarrow commented 2 years ago

@AndrewLock, Does this PR give enough flexibility whilst keeping the spirit of the library?

From: BOBJohnson @.> Sent: 15 March 2022 20:21 To: andrewlock/StronglyTypedId @.> Cc: Sean Farrow @.>; Author @.> Subject: Re: [andrewlock/StronglyTypedId] [Proposal] Add the ability to optionally provide an implicit conversion operator between the wrapper type and the primitive type (#39)

@.https://github.com/BOBJohnson/StronglyTypedId/commit/53339c06cdc59e36f155892cb300ed143499a871 (original commit) @.https://github.com/BOBJohnson/StronglyTypedId/commit/7e4eb19ab8d14f708e25cb6229e8d2e1035fcb28 (revised commit)

If you get a chance, please review my fork + commit in reference to this proposal.

One thing I haven't done is updated any of the tests as I felt that was outside my wheelhouse with the limited understanding I have of your project.

I also didn't update the readme.

Considering that I haven't updated the tests (I did manual testing though with a test project and local version of package), nor updated the readme, I didn't feel it was ready for a PR just yet.


Original commit added new values to the StronglyTypedIdImplementations flags enum but I didn't like the fact you could specify mutually exclusively flags with how I had done it originally.

Revised commit split it out into a new StronglyTypedIdConversionOperator enum that had 3 values (None, Explicit, and Implicit). You can then specify the type of conversion operator from the base type and a conversion operator to the base type.

IE, you can now do: [StronglyTypedId(fromOperator: StronglyTypedIdConversionOperator.Explicit)] public partial struct MyId {}

This now allows the following code: MyId myId = (MyId)someGuidVariable;

— Reply to this email directly, view it on GitHubhttps://github.com/andrewlock/StronglyTypedId/issues/39#issuecomment-1068431366, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AALDK7RIB64E37ZRTPQG5E3VADWLXANCNFSM5DLIL54A. You are receiving this because you authored the thread.Message ID: @.***>

lecaillon commented 2 years ago

Personally, I'm quite strongly against this one, it removes one of the big safety guards that strongly typed ids bring.. That said, having it as an option doesn't seem too harmful..

I guess lot of us here use a strongly type language called C# that let us use the dynamic keyword if we decide to ;) Let the developer choose what is best for him depending on his context. Add an option seems a very good idea to me :)

MovGP0 commented 2 years ago

Such conversions can ease migrating existing code and should be an opt-in feature.

When enabled, the conversion from primitive type to ID type should be implicit, while the conversion from ID to primitive type should be explicit.

andrewlock commented 2 years ago

@BOBJohnson I'm ok with having the operators enabled via an option, but I'm not sure I would go with the approach you have in your fork. I think I would opt to have the conversions as StronglyTypedIdConverters instead of adding additional properties 🤔

When enabled, the conversion from primitive type to ID type should be implicit, while the conversion from ID to primitive type should be explicit.

I'm inclined to agree with @MovGP0's assertion here, which would simplify the implementation too (only requires two additional options in StronglyTypedIdConverter

BOBJohnson commented 2 years ago

@andrewlock Whatever you think is best. I only poked around in your code and only had a very passing familiarity with it. I did make them optional parameters, but when not optional, they do make the Attribute tag a bit on the long side.

Never worked with a source generator before so it was also just a learning exercise for me.

andrewlock commented 10 months ago

I think I've found an answer to feature requests like this this in the big redesign of the library in this PR:

The main idea is to make the library much more maintainable while also giving people a mechanism to customise the generated IDs as much as they like. This will make it easy to make changes like this as you see fit 🙂