Move methods handled in TryInvokeMember to their own methods
FransBouma opened this issue · 7 comments
The method TryInvokeMember is currently handling way too much and the interface of the method is very limiting, so adding additional elements to the method, like arguments for a where clause, leads to problems which are hard to solve.
If each method which is currently handled by the method is moved to its own method like we have now as e.g. Single, it's more manageable and each method can then also receive the right arguments properly.
See #279
TryInvokeMember is obscure. It removes IntelliSense from DynamicModel for some, but not all, of it's possible calling patterns - which means that I think those patterns are bound to be less used by someone new to Massive.
On the other hand, it achieves the Massive goal of producing a lot of functionality from a small amount of carefully thought out-code. Removing that would expand out the size of Massive - again going against one if its original stated goals.
So, I'm not sure - IntelliSense is nice! But I did just want to make that preceding point, along with the point that, based on the discussion in #279, I believe there is a correct (non-hacked) way to support args for where in TryInvokeMember, if someone wanted to. You'd need to use actual parameter names e.g. @name for name:value pairs. Then numerically named parameters could be reserved only for parameters in args, which means no mock-parsing of the whereclause any more. I think that would actually work. Another line of fix would be to do this, along with beefing up the documentation about what TryInvokeMember member can do for you, in README.md.
Also, is it right that removing TryInvokeMember would remove the support for name:value pairs, or am I getting confused?
TryInvokeMember shouldn't be removed, it's there for a reason: all method calls on a dynamic which aren't implemented as normal interface methods of the class will ending up there, like in Ruby. This issue discusses the move of the methods currently handled by the method as such, i.e. the ones which are named there. It still allows after that a random call to a random method to end up there.
On the other hand, it achieves the Massive goal of producing a lot of functionality from a small amount of carefully thought out-code. Removing that would expand out the size of Massive - again going against one if its original stated goals
Not sure what you are referring to. Adding features all cramped into TryInvokeMember is not the way to go. Some left-over things can be there but in general the methods should have their own implementation. Adding methods separately would increase the filesize, but cramping it all into one method with very limited functionality isn't great either, so the methods ideally have to have their own implementation.
What you suggest might work, but it's very brittle: users have to make sure they get everything right otherwise it will break at runtime, which isn't ideal. It requires users to read the docs extensively. People don't do that, even though it will save them time, sadly.
For the reason of adding IntelliSense, and not having to get everything right before it gets tested at runtime, I agree (for what it's worth - maybe not much) that I like the idea of splitting off these methods.
Can I just ask - I know you're probably going to end up doing it, and probably have it clear already - how would name:value pairs be supported in the split off versions? I think I've got it, actually, is the idea that TryInvokeMember would still support all these calls as before, and with name:value pairs where needed, but would no longer be what handled the calls in the simpler cases without name:value pairs?
I wonder if TryInvokeMember() could be refactored to look for an explicit method to call, after doing only pre-processing to support named arguments. That might be a neat solution? (It would ensure that the TryInvokeMember variants and the explicit variants couldn't drift apart in functionality, and would ensure there was IntelliSense for everything except name:value pairs.)
That still suffers from the limited interface TryInvokeMember offers. I don't think it's really a great interface for things other than name:value pairs: making these more diverse begs the question why not add methods for that.
I once thought it could be used to have it call stored procs, like
var foo = myDB.SomeProcName(param1:value, param2:value);
as that's the true benefit of dynamic programming also in ruby: you call a method which isn't there yet, but the catch-all method can execute it. In massive it then would mean it would use the name of the method for a procedure name and pass the name:value pairs as parameters. Very limited! but it reflects the way dynamic works. How TryInvokeMember is currently implemented, it's taking care of missing methods which should be there: they are fixed methods of the API (as they're hardcoded in the code TryInvokeMember!). A flexible method handling like for procs like I illustrated above, is its true purpose as it can handle a changing api, namely the one in the DB.
I didn't pursue with it as it was too limited.
That does sound clever, and useful as far as it goes, but still too limited, I agree.
Re my previous comment, I am thinking that if you split the methods out but keep TryInvokeMember more or less as is, then it may make the new code design confusing, because TryInvokeMember will then actually do exactly everything that the split-out methods do, plus more (i.e. plus name:value pairs). (Everything except IntelliSense and code readability, that is.) Someone might notice that you could delete all the factored out methods, and that the code would still compile and support exactly the same calls (barring the difference that it would only call correctly from dynamic types), and then wonder what was going on... I was thinking that if TryInvokeMember could be refactored so that it adds name:value value pair support, but only to otherwise explicitly existing methods, then this would make its purpose easier to understand.
Full disclosure: I was thinking that since adding parameter name and direction support to AddParam is possible and quite easy (as I did for SQL Server in my SP pull request, and since all db's parameter work is currently done with non db-specific types, this could quite easily be correctly refactored and done for all DBs), that it would therefore be quite easy to make TryInvokeMember produce a bag of named parameters (with @0 @1 as the names for any params from args, and @name for any params from name:value) which it could pass on to the other methods. Just a thought...
Just looking at the code again, I reminded myself that TryInvokeMember is written so that FindPartridge, GetBalance, LastZebraOnEarth, SingleWhiteFemale will work as aliases for Find, Get, Last, Single, etc.
My hesitant proposal of refactoring TryInvokeMember (above) - and even the general proposal of splitting out these methods (i.e. this Issue as a whole) - obviously can never support all that.
The easiest way to continue to support 'all that' after splitting off the methods would be to leave TryInvokeMember as it is - but I think that carries the genuine risk that the newly split off methods eventually diverge from their near equivalents in TryInvokeMember. (As well as the arguable risk of it being confusing to understand why Massive supports all these calls in two different ways at once.)
The best way to deal with the fact that Massive supports 'all that' clever naming might (indeed) be to stop supporting it? Partly because it seems overly complex and unnecessary; partly because it makes it impossible to produce a revised TryInvokeMember which focusses just on doing what it really does for the project (which is add name:value support). But that could obviously only happen at a future major version.