search

mikhailshilkov / With.Fody

A Fody addin to extend immutable C# classes with With() methods to return a copy of an object with one property modified.
MIT License
36 stars 3 forks source link

Relax the type checking #14

Open RaringCoder opened 6 years ago

RaringCoder commented 6 years ago

Hi. I started using this the other day and must say it makes working with my DDD aggregate state objects much nicer. My only gripe is the initial checking between properties and the constructors available.

It feels too strict limiting the types to matching types. For example, I expose an ImmutableArray<T> but my constructor takes IEnumerable<T>, as this is the only contract required during construction. These constructors are therefore not found by the GetValidConstructor method. I believe the With method should type-match the property, as it's meant to mimic the F# style, however it doesn't need to be too prescriptive on the constructors used.

One that I like is to stipulate that the type needs to have a constructor that has arguments for all properties and the parameters should share the property name, no more. Type issues can be left to the developer. We could do type checking, but I would suggest checking that the property is assignable to the constructor argument (as the property might be derived from the argument and the constructor translates it) or that the argument is assignable to the property (the property might expose an IEnumerable<T> where the constructor takes a T[]). The With should match the constructor.

What are your thoughts on this?

mikhailshilkov commented 6 years ago

Hi. Great to hear that you find it useful!

I think the idea to relax the check to IsAssignableFrom makes sense. Fancy making a PR?

RaringCoder commented 6 years ago

Yeah happy to. I'll go with the approach of checking assignable in both directions (so that it doesn't depart too much from what's sensible).

Another use case I will cover is Nullable<T>, which has just stung me. I have a DateTime? on my state object, but due to the domain it is not valid to set null, only a real value, from public code.

RaringCoder commented 6 years ago

Evidently type walking in Cecil is odd. I can do sub-class or interface checks, however arrays start to go a little funny (I guess because of all the special compiler-level support they get). This could degenerate into a complete mess of code to handle edge cases.

I then tried to fall back onto reflection, but this gets a little messy trying to load types in the correct assemblies.

Edging towards just doing it by name and letting the runtime complain if developers want to do stupid things lol