-
Notifications
You must be signed in to change notification settings - Fork 322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move methods handled in TryInvokeMember to their own methods #287
Comments
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 Also, is it right that removing |
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.
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 |
I wonder if |
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
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 |
Just looking at the code again, I reminded myself that 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. |
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
The text was updated successfully, but these errors were encountered: