search

FransBouma / Massive

A small, happy, dynamic MicroORM for .NET that will love you forever.
Other
1.8k stars 323 forks source link

Query and QueryAsync DbConnection versions are never used #303

Closed mikebeaton closed 7 years ago

mikebeaton commented 7 years ago

Query and QueryAsync have variants which accept a DbConnection.

These can be completely deleted from Massive and the project compiles successfully and all tests on all databases run successfully.

I realise that since these methods are there, some poor soul will have used them, and they can't just be removed. But... NOTHING else in Massive exposes any public method which accepts a DbConnection (as I now realise, c.f. #301 #234). Is it a mistake, originally, that these exist? Are they needed for some subtle reason that you know of? Thanks.

FransBouma commented 7 years ago

Looking at the v1.0 branch, they've been there since the beginning: https://github.com/FransBouma/Massive/blob/v1.0/Massive.cs#L206 I agree it's a bit inconsistent at places, as it begs the question why is this method exposing a connection but no other methods, and why is it exposing a connection but not a transaction? etc.... but unless @robconery gives us insight in the reason of this method to be there, we'll never know ;).

It's a public method, so it's kept as-is in v2.0, and I just created an async variant of it. That all tests succeed is not a surprise btw, in v1 there were no tests so I wrote tests for the features that were exposed by Massive to see that they indeed worked. It doesn't have 100% coverage.

robconery commented 7 years ago

I believe they were put in because someone (maybe me?) wanted to be able to query multiple databases.

mikebeaton commented 7 years ago

Can query multiple databases (of same basic type, i.e. Oracle/SqlServer/etc.) with multiple DynamicModel's now... so possibly could be marked as obsolete?

FransBouma commented 7 years ago

Nothing in the public API is marked obsolete nor removed. Your proposal is a change just to make a change, it doesn't solve any real problem, as there is no problem.

mikebeaton commented 7 years ago

I don't agree.

Massive has various consumers, including people who might want to add to or work with the code. Massive has always been a codebase which advertises itself as something you can pick up and modify, if you need to. Having a misleading, inconsistent, non-required part of the API (even in a quiet corner case) does not serve that constituency so well.

That is why I mentioned it.

The case is closed. You are not going to change it. That is fine. But it is not correct that I am proposing changes just for the sake of it.

FransBouma commented 7 years ago

I'm sorry but I get a little tired of defending myself in every issue in my own repository. The thing is: massive is out for a long time, people use it in a lot of different situations. This means that stability is key: every change made to the public API has to be justified, e.g. it fixes an issue which is otherwise not fixable. However that's not the case here: it's just a method which happens to be a public method and perhaps it should have been better if it would have looked differently. The fact that it is part of the public API means it can't be changed though, and it can't be removed. Stability, backwards compatibility, dependability.

Marking it as obsolete fixes no issue which isn't fixable otherwise, it will only cause problems for existing users. I have no idea why on earth I would make that change. As there's no other reason for it than that the method might not look in line with the rest (which is true, but irrelevant, as removing it is out of the question), there's no reason to make any change.

Sorry but this is just a solution looking for a problem, and now a debate about that. While there's actually no problem that needs fixing. That's the aspect of maintaining software that's been used by many thousands of people for a long time: you can't change things that will break their dependency, you can't move your cheese.

mikebeaton commented 7 years ago

Okay, I understand that.

It is all very well for me to turn up and say, "What would be great for Massive, so that someone who next comes across it will find that it does everything which it already does, plus a few more genuinely useful features?"

(Let's say, at least, that I do think that the full, cross-database stored procedure support which I have now coded - and which I use every day in my own work - is a genuinely useful feature. I am less sure if it can be made into a well-integrated, non-breaking addition.)

Anyway, it is quite another another question to ask, "What is worth adding to Massive, and how should it be done, if the first and most important condition is to continue to make Massive work stably, exactly as it does, for all the thousands of people who now depend on it every day?".

I do fully see - though only after you have made it clear to me, sorry - why it is absolutely crucial that you do do that.