Everything needs to be public and virtual!!!

[PaulDMendoza]

Please make all methods and classes public and all methods marked with virtual so I can extend them easily. There are tools you can use to do this automatically for you at compile time I believe. This is an API and I don't want to compile my own version so I can get to the definition of Dapper.SqlMapper.DapperRow for instance. There isn't any reason to make things private since all the source code is open source.

[mgravell]

Accessibility isn't about whether the code is OSS. It is about logical encapsulation. Polymorphism is a lot more than "just make it public and virtual" - it needs to be designed, implemented and tested for such. "Public/protected by default" and "virtual by default" are terrible ideas. In the case of DapperRow, this would immediately prompt the question: "and how does your subtype ever get instantiated?". It isn't as simple as just unsealing the type and making some things virtual. Heck, the fact that it is explicitly sealed should hint strongly that this isn't an intended use-case.

So: step back a second: what is it that you are trying to do? On 14 Sep 2014 01:01, "Paul Mendoza" notifications@github.com wrote:

Please make all methods and classes public and all methods marked with virtual so I can extend them easily. There are tools you can use to do this automatically for you at compile time I believe. This is an API and I don't want to compile my own version so I can get to the definition of Dapper.SqlMapper.DapperRow for instance. There isn't any reason to make things private since all the source code is open source.

— Reply to this email directly or view it on GitHub https://github.com/StackExchange/dapper-dot-net/issues/166.

[isidore]

Hello,

Paul asked me to share my story of how & why I came to appreciate making my OSS project more extendable.

I got a bug reported about TFS build system producing weird behavior. It was a rather 'undefined' type of bug that I particularly hate and Installing TFS is the kind of particular hell I prefer to avoid.

So I need to do some troubleshooting and avoid installing the system. Fortunately, I tend to remote pair with my users (skype and join.me) so I could do all of this just be working on their computer. (I should mention that this has helped me develop a lot of empathy with my users which has also been helpful)

So, I ring up Blake and we work on his machine and he shows me the error. It's confusing, so we start writing code to try to trouble shoot the error. Quickly we run into trouble because I can't change the part that I need to touch. It's private and the accessors are not virtual. This is because from a design perspective I shouldn't need to touch these areas. But, of course, I do need to touch these areas because something out of the scope of my design is happening.

I can't tell you how frustrating it was to not be able to fix my own code! I had to manually change the code on my machine to be public/virtual and then upload the myget version so we could continue.

This is where the "free as in freedom" part of OSS started to occur to me. Nobody wants to customize my code. Nobody wants to extend and change my code. They want my code to work. They are changing it because it doesn't work for them

The biggest problem with non-virtual public items is it requires you to plan ahead to allow for something that you haven't planned for. Which has a bit of a catch 22 issue.

btw: you can see this in your question, "what is it that you are trying to do?" You shouldn't have to have a conversation with the creator of a product to solve your problem. That conversation should be an added bonus when it's too hard to figure out or when people are contributing back to the project.

I think I should also mention the details of how I did this. C# isn't the nicest language for allowing others to monkeypatch. I also care about the design of my project and the use of public/private/protected/virtual all say something to the writers of the code (ME). Nonetheless, I care about my users being able to use my code. I don't want to sacrifice either side. Fortunately, Simon Crop's Fody is a nice weaver that solves this at compile time. The nuget command is

Install-Package Virtuosity.Fody

It adds a small xml file with

To the dll's you want it to change at compile time. Easy wins for both you & your users.

Hope this helps!

[devlead]

I agree with @mgravell, exposing too much would increase test surface and could impair future featuring refactoring. Speaking of OSS if you need full access just link in the dapper class, then you have full access&responsibility. I do this with help of the T4Include Nuget, this gives me source code level access with DLL like "stability".

[mgravell]

Again, polymorphism is only useful if you end up with your custom sub-type; that won't happen just by making some methods virtual. It is neither correct nor reasonable to assume all methods should be virtual. I am asking what the use-case is so that I can understand what it is you want to be virtual, and why. So that I can a: understand whether this is an appropriate thing to do, b: understand what would need changing, and c: understand how to test / validate such.

As a library author, it is entirely my discretion what methods are virtual in the master/trunk. This isn't about being petty - it is about understanding the maintenance cost. Every virtual method is something that a: adds support overhead, and b: limits the flexibility to change implementation. It is therefore essential to understand the use-case to see what the impact of making it virtual is. It isn't zero cost, and I'm the one who does all the paying...

[mgravell]

BTW - Anders talks about precisely the issue of too much virtual here: http://www.artima.com/intv/nonvirtualP.html

I genuinely don't object to making changes to support scenarios that I haven't previously considered. But designing any system does require at least a high-level understanding of what the use-cases are that it needs to support. That is what I am lacking here: a use-case. On 14 Sep 2014 16:33, "Mattias Karlsson" notifications@github.com wrote:

I agree with @mgravell https://github.com/mgravell, exposing too much would increase test surface and could impair future featuring refactoring. Speaking of OSS if you need full access just link in the dapper class, then you have full access&responsibility. I do this with help of the T4Include Nuget, this gives me source code level access with DLL like "stability".

— Reply to this email directly or view it on GitHub https://github.com/StackExchange/dapper-dot-net/issues/166#issuecomment-55529203 .

[isidore]

I can only provide my datapoint as an example, but I have had no extra cost of in maintenance cost. Everyone that went and customized a monkeypatch realized that have gone into the 'unsupported' area.

I have had less maintenance issues for two reason: 1) Many people are empowered to solve their own issues 2) Some of those people then submit a good use case/solution to me.

Again, this goes to the "free as in freedom" The fact that you are saying "I not going to approve a change that lets you solve your problem until you convince me that the particular method of solving your problem is the manner in which I agree with" is an unfortunate.

and, I'm not holding back a use case form you, I honestly have no idea what paul is trying to do. I just believe that he should have the ability it try things.

[PaulDMendoza]

Marc, Thanks for responding. First, let me say Dapper is one of the greatest changes to querying data that I've experienced in my 10 years programming .NET. I've now abandoned entity framework as my solution going forward and my motto is "just write SQL and make JSON".

This request was somewhat inspired by an API design discussion/talk given by Llwellyn (isidore) which seemed very right.

Here are two use cases where public by default would have helped me which I eventually figured out but where public would have helped to not have to go to the source code. No one wants to read your source code.

1. The Query() function returns a collection of dynamics which underneath are actually Dapper.SqlMapper.DapperRow object types. The Dapper.SqlMapper.DapperRow is private. I needed to dynamically add properties to the Dapper.SqlMapper.DapperRow objects but that doesn't appear to work. As a result I wanted to convert the Dapper.SqlMapper.DapperRow into an ExpandoObject. I would have loved to have done a cast from dyanmic to Dapper.SqlMapper.DapperRow but since it is private I couldn't even name the type in my code. Using the debugger was how I found out that the Dapper.SqlMapper.DapperRow objects were being returned. I looked at the Object Explorer for Dapper and couldn't find the class Dapper.SqlMapper.DapperRow. I thought something was wrong with Visual Studio so that took 15 minutes to track down I wasn't crazy because I hadn't yet discovered it was private. Then I went searching through the Dapper source code for the DapperRow class and after 30 minutes found what I was looking for. What I really needed to know here was that Dapper.SqlMapper.DapperRow implements the interface for IDictionary<string, object>. Overall I probably spent an hour and a half head scratching before I had the solution. I was able to build this generic helper method like below.

   public class DapperHelpers
   {
       public static dynamic ToExpandoDynamic(object value)
       {
           IDictionary<string, object> dapperRowProperties = value as IDictionary<string, object>;
   
           IDictionary<string, object> expando = new ExpandoObject();
   
           foreach (KeyValuePair<string, object> property in dapperRowProperties)
               expando.Add(property.Key, property.Value);
   
           return expando as ExpandoObject;
       }
   }

2. Another great example of where "public" would have helped was the "isConsumed" variable on the return object from QueryMultiple(). Billy Ramos and I needed this property and it was already on the object and was being set, we just needed to access it. We ended up downloading the Dapper source code, removing the nuget binding from our project using Dapper, compiling our own version and using that. Then we submitted a pull request which you implemented and then you published to nuget . We now have a task to go back to using the nuget feed for fetching Dapper. For us it would have been a 3 minute fix had that variable been public. Shame on Billy and I if we don't then submit a pull request but still, we wouldn't have had to remove the nuget binding.

The virtual request was more of "nice to have" and "why not". I don't have a perfect use case at the moment.

Thanks for taking the time to respond Marc!

[mgravell]

It is absolutely "free as in freedom". You can absolutely take a fork and do whatever you want, without any obligation, cost or prohibition. Stallman would be happy. For something that goes in the library: exposing virtual methods is basically offering an additional API contract: it very much adds a maintenance cost and a refactoring restriction - in that in the future the library needs to either honour that same extension point, or publicise a nasty breaking change. As such, any virtual members need to be duly considered in terms of what that means in the future. I cannot consider that unless someone indicates what members they want to be virtual (and ideally, for what family of scenarios). And again: the required change is not simply to make some members virtual: I can say with absolute confidence that this by itself would achieve nothing useful.

I'm happy that you've never experienced maintenance costs from blindly making members virtual. I have experienced that cost. On 14 Sep 2014 17:07, "Llewellyn Falco" notifications@github.com wrote:

I can only provide my datapoint as an example, but I have had no extra cost of in maintenance cost. Everyone that went and customized a monkeypatch realized that have gone into the 'unsupported' area.

I have had less maintenance issues for two reason: 1) Many people are empowered to solve their own issues 2) Some of those people then submit a good use case/solution to me.

Again, this goes to the "free as in freedom" The fact that you are saying "I not going to approve a change that lets you solve your problem until you convince me that the particular method of solving your problem is the manner in which I agree with" is an unfortunate.

and, I'm not holding back a use case form you, I honestly have no idea what paul is trying to do. I just believe that he should have the ability it try things.

— Reply to this email directly or view it on GitHub https://github.com/StackExchange/dapper-dot-net/issues/166#issuecomment-55530160 .

[devlead]

@pauldmendoza T4Include by @mrange would solve that as you would compile source with your project you could step thru it yourself. This without changing Dapper API and without needing to fork.

Read more here: https://github.com/mrange/T4Include

Just like adding i.e. optional parameters, changing to virtual/public can cause unforeseen effects and increase surface area needed for testing.

[mgravell]

Note that T4Include refers to an old (incorrect) location. It also does not include the .NET 4.5 async code (a separate file). On 14 Sep 2014 18:01, "Mattias Karlsson" notifications@github.com wrote:

@pauldmendoza https://github.com/pauldmendoza T4Include by @mrange https://github.com/mrange would solve that as you would compile source with your project you could step thru it yourself. This without changing Dapper API and without needing to fork.

Read more here: https://github.com/mrange/T4Include

Just like adding i.e. optional parameters, changing to virtual/public can cause unforeseen effects and increase surface area needed for testing.

— Reply to this email directly or view it on GitHub https://github.com/StackExchange/dapper-dot-net/issues/166#issuecomment-55531829 .

[devlead]

Well a new NuGet needs to be published by @mrange, have an PR accepted with the correct one. Then again Dapper being in the default template is more of an example, you can cherry pick from anything on GITHub.

[mrange]

As @devlead pointed out the dapper sample is just a sample and you can include whatever. With that said I should publish the updated version ASAP.

WRT to OP. Just blindly changing everything to virtual and public is IMHO undesirable.

1. It likely doesn't give the correct granularity.
2. Objects should probably be constructed using a DI Container to really reap the benefits but that increases footprint of dapper and can impact performance.
3. The real kicker is that regression testing will be much more difficult in the future as Dapper then can't remove methods that someone might have overriden or change the behavior even in subtle ways because someone might depend on that specific behavior.

[isidore]

Wow, the amount of empathy shown toward your users here is simply amazing :-(

[devlead]

Empathy...wow. @mgravell made a design decision, motivated why. Others have offered alternative solutions that would work around the problem. I'm sure you will get a full refund ;)

[mgravell]

Thank you for making a technical design choice into a personal attack. Although to be fair, it isn't as though I repeatedly explained why the proposed solution could be detrimental to the project and therefore all users. It isn't as though I repeatedly took the time to try to find out what the actual requirement is (the problem, not the proposed solution), and then when I finally got that, agreed to investigate. On wait, yes, I did those things.

Not immediately agreeing to implement a (bad, IMO) suggestion without first investigating what the actual problem was - is not a question of empathy. Thank you for your support and appreciation of the work and dedication that goes into a project like this.

[mgravell]

Ah, I wonder it this is related to your angst, @isidore - this is in my sent items from 2 days ago, but does not appear in this chain - looks like github didn't process the inbound for some reason:

On 1:

Yes DapperRow is currently private; since it maps to the dynamic scenario, it is not hugely expected that you would need to know the name - and in fact it has changed implementation at least once. The fact that it also implements a particular interface is something that should probably be better documented - I can fix that. I can also investigate the "add members" scenario. There is nothing in here that shouts "polymorphism" as the appropriate answer, nor "make the type public". The use-case seems to be "I want to add custom members afterwards". So let's consider that!

On 2:

That is precisely what pull requests are for. I apologise for not exposing this originally, but I am not perfect. It sounds to me like the OSS model worked perfectly. There's no way I would have exposed the field publicly; none of my integration tests / use-cases required that property (there is a lot you can do without ever needing it); therefore it didn't get added. Somebody later did need it: it got added and tested.

[mgravell]

@PaulDMendoza feedback on those On 1 / On 2, please, before I dig into the code.

[mgravell]

The documentation has been improved to clarify the APIs available; I can't help but conclude that this covers the most useful parts of this.