microsoft / kiota-abstractions-dotnet

Abstractions library for the Kiota generated SDKs in dotnet
https://aka.ms/kiota/docs
MIT License
24 stars 21 forks source link

Models should be partial classes #231

Open MihaMarkic opened 2 months ago

MihaMarkic commented 2 months ago

Models should be partial classes, so one can extend them if necessary.

baywet commented 2 months ago

We've received that suggestion a couple of times and rejected it for the following reasons:

When digging into why they'd like to extend the models, the main reason was "OpenAPI description is inaccurate/incomplete". Can you provide specific scenarios this would enable outside of that main reason?

MihaMarkic commented 2 months ago

One reason would be to implement #232. Other reason could be to implement a combined property getter - such as IsValid => SomeProperty == Something && OtherProperty == SomehtingElse or ToString() etc. I was thinking more in augmentation direction than fixing generated code.

baywet commented 2 months ago

Couldn't you do that through an extension method?

MihaMarkic commented 2 months ago

I can't implement interface nor create properties through extension methods. True, I could create IsValid() extension method in such case. Anyway, partial classes give more flexibility. At least you might consider generator flag that would toggle partial keyword inclusion.

darrelmiller commented 2 months ago

We are not strongly against partial classes, we just need clear use cases to justify the work and the additional conceptual complexity.

baywet commented 2 months ago

as for additional flags, not having any is a design philosophy for kiota. because it leads to additional complexity on the CLI experience, and additional maintenance burden. We've already been asked for a dozen different flags...

MihaMarkic commented 2 months ago

as for additional flags, not having any is a design philosophy for kiota. because it leads to additional complexity on the CLI experience, and additional maintenance burden. We've already been asked for a dozen different flags...

Fair enough.

MihaMarkic commented 2 months ago

We are not strongly against partial classes, we just need clear use cases to justify the work and the additional conceptual complexity.

IMO there is almost no work adding keyword partial to all models. As per complexity people will make out of it - even now is possible to manually add partial keyword where required - albeit its tedious, or modify generated code. My main use case today is implementing #232 which would prettify and reduce my code. Basically applying an interface. Then (from my past experience) imagine models that share some traits, i.e. CreatedDate property. One could slap another interface to these and uniformly work on them instead of using plenty of _if_s. I'm sure there are other possibilities with generated partial classes.

baywet commented 2 months ago

I believe that change is simple enough and the use case of "I want my models to implement an extra interface" can only be enabled this way that we should consider it. This would be limited to dotnet though. Thoughts @darrelmiller ?

baywet commented 1 month ago

ping @darrelmiller for confirmation before we proceed forward with a pull request here