Specification
Specification copied to clipboard
ardalis
397 make fields and methods protected
If you use a property with private set; doesn't that still limit extensibility since there is no way to replace a dependency with a different implementation? Sure, readonly access is better than no access, but I thought the intent of #397 was to allow the replacement of the dependency if desired...
For my needs, I only needed to override the methods while still being able to refer to and read the fields (now props) from the subclass. I don't need to mess with the constructor signatures or replace/modify the state. Only the behaviour.
So for me, this is enough. I can see a case for opening it up further. If you believe this is the way to go, I'll happily remove the private markers 😊
I'm not clear on what the benefits of the property are over the field, in this case. The field change (as originally done in this PR) seems simpler and provides greater extensibility. The property adds additional encapsulation, but I'm not sure that's needed or desirable in this case.
However, I recognize I'm not the target audience for this change, so if y'all are ok with it as is I think we can merge it. Let me know.
In that case, let's opt for maximum extensibility with maximum encapsulation. Most power and most safety. I've removed the private keyword to make the properties protected on both get and set
TL;DR: Holy 💩, I didn't mean to start such a conversation about a property 🤣... Any solution allowing te behavior described here is fine for me.
Longer version
Protected properties with private sets look like the cleanest way to create RepositoryBase<T> children with custom behavior. Personally, I'm not a big fan of field inheritance and I always saw this as a code smell (thus my suggestion for the property).
As per @ardalis observations about extensibility, I opened #397 mostly because I needed a way to "override" the auto-save behavior in the methods and therefore, as @eldamir correctly pointed out, you need to access the DbContext somehow. Furthermore, having injected the generic DbContext trough the constructor provides enough flexibility even for scenarios with multiple contexts.
I think we landed in the best spot here. No "field inheritance", and properties are available for read and write as needed by inheritors with proper encapsulation. I'd be happy to see this merged 😉
