andrewlock / StronglyTypedId

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

Generated code doesn't implement some comparison operators #56

Closed bbrysh closed 9 months ago

bbrysh commented 2 years ago

The id structs generated don't implement some comparison operators (<,>, <=, or >=.) The CLR cannot automatically call the CompareTo implementation from Equals(object) or from the base comparison operator implementations.

andrewlock commented 2 years ago

Thanks, I specifically didn't add those previously, as I didn't want them. For IDs, those comparisons don't make any sense. It seems reasonable to make optionally add them though I think

bbrysh commented 2 years ago

The optional route sounds good to me. This would support a scenario like someone is searching for an integer id using a binary search of a sorted list. It also helps me get rid of some SonarQube warnings :)

SteveDunn commented 2 years ago

@bbrysh - you could also implement IComparable yourself, as the classes are partial.

bbrysh commented 2 years ago

I could, but to me that defeats the purpose of using the library - not writing boilerplate code. Also, it's a good idea to treat IComparable like IEqualable. If you implement the IEqualable interface, you should override the equality operators. For IComparable, you should override the comparison operators. The templates already override the equality operators, so a precedent has been set.

SteveDunn commented 2 years ago

I would say that checking equality of IDs is much more common than sorting IDs. Granted, sorting could be useful, but whether or not that fits with a general purpose ID library, I'm not sure. I'm toying with the same idea in my library, Vogen, which is similar, but focused more on 'Valuen Objects' and how to ensure you can't create default/invalid instances through the use of code analysers and compilation errors. Trouble is, the next logical step would then be implicit operators to/from the underlying type, at which point, you risk losing type safety.

https://github.com/SteveDunn/Vogen

On Tue, 8 Mar 2022, 18:05 bbrysh, @.***> wrote:

I could, but to me that defeats the purpose of using the library - not writing boilerplate code. Also, it's a good idea to treat IComparable like IEqualable. If you implement the IEqualable interface, you should override the equality operators. For IComparable, you should override the comparison operators. The templates already override the equality operators, so a precedent has been set.

— Reply to this email directly, view it on GitHub https://github.com/andrewlock/StronglyTypedId/issues/56#issuecomment-1062058773, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACAJ6C4FYJMD4OP2NG7VELU66JHJANCNFSM5PR4VCXA . You are receiving this because you commented.Message ID: @.***>

bbrysh commented 2 years ago

I don't believe that implicit operators would be needed as the Value property is the underlying typed value. I think having the overloaded comparison operators is a good choice as nobody knows all the cases where this library might be used. Since IComparable is an optional part of the ids generated, when the interface is generated, the operators could also be generated.

MovGP0 commented 2 years ago

For IDs, those comparisons don't make any sense.

I disagree. Ids often need be sorted for some reason. Especially with ids of type byte, int, or long.

timbze commented 1 year ago

== and != is not mentioned in OP, but that's what I mostly need for ID. I'm doing it manually for now but sure would be great to have the option to generate those!

cyril265 commented 1 year ago

I'd like to add that > and < operators for ids are needed if you want to implement cursor based pagination. Your query would be something like that: db.Entity.Where(e => e.Id > previousId).OrderBy(e => e.Id)

andrewlock commented 9 months ago

I'm slow to get to this, but the fundamental point of "if you implement IComparable you should implement the operators" is sound.

I've done a redesign of the library in this PR, and added the operators in that 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. Which will make it easy to make changes like this in general.