Add invocation helpers for (generic) methods (on generic types) #89
Conversation
|
Dang, that's more code than I expected. At a glance it doesn't look like any of your classes are public. Not sure if that's the intent? Anyways, I'll take a more in-depth look a bit later. |
There was a problem hiding this comment.
Missing docs/examples, which IMO makes having these as shared utilities in NML for mods pretty pointless.... Though I guess NML's API doesn't have proper generated docs to begin with, but IDEs usually can at least show the special C# doc comment XML monstrosities...
Overall I can at least say that I wouldn't be able to maintain this code as I have no real idea of what it does, other than some funny dynamic invocations & seemingly caching... Not that that should block it from being merged if others find it useful & understand it ofc
As I said in the original post, I wanted to get people's feedback on the capabilities / interface before spending time on documenting it extensively. However if you have any questions as to what it does I'd be happy to explain / go on discord |
takes off C# developer mask
I think the most helpful thing here would be an example or two on what a typical usage of this API would look like. No need to go crazy with the scary XML comments yet... especially because I'm pretty sure like 95% of mod devs don't actually know how to see the XML docs in their IDE. At least that's the vibe I get with some of the questions asked on the Discord. |
...usually just hovering over the methods should show a popup of the details if the comment is formed correctly.
There's a few choices, though only one out of these that I've used before is doxygen... DocFX looks more modern though from a quick investigation. |
Here's one for the GenericMethodInvoker: https://github.com/Banane9/NeosDynamicVariableLogger/blob/fa0f21ee2cf0c50af83770c47448a54cea2febc9/DynamicVariableLogger/DynamicVariableLogger.cs#L132 I've used it some more in the CustomEntityFramework to help with the invocation of Methods<> on DynamicVariableSpace and while patching DynamicImpulseTriggerWithValue<>, but @KyuubiYoru still need's to set that repo to public. |
that only works in the same project, otherwise you need to enable the Documentation file option under Build > Output in the Properties and have it along side the file. Generating the pdb information (and embedding it?) may also be a helpful idea (for a dev version anyways). |
|
Here's another example of the GenericMethodInvoker: https://github.com/KyuubiYoru/CustomEntityFramework/blob/a9791a41380ff46b8de2bde5e586f291555d108c/CustomEntityFramework/Functions/DynamicVariableSpaceWrapper.cs#L208-L239 And of the GenericTypeMethodsInvoker: https://github.com/KyuubiYoru/CustomEntityFramework/blob/a9791a41380ff46b8de2bde5e586f291555d108c/CustomEntityFramework/Functions/CustomFunctionLibrary.cs#L130 |
| //if (checkType && hayParameter.ParameterType.FullName != needleParameter.ParameterType.FullName) | ||
| // return false; | ||
|
|
||
| // TODO: Do a proper type check? lol |
| return InvokeInternal(method, instanceTypes, null, new TypeDefinition(), parameters); | ||
| } | ||
|
|
||
| private static MethodInfo GetGenericMethodOfConcreteTypeDefault(MethodInfo needleMethod, Type concreteType) |
There was a problem hiding this comment.
There are a few things I'm confused about.
- Wouldn't something like this be equivalent?
AccessTools.Method(needleMethod.Name, needleMethod.GetParameters())Or is there something I'm not seeing? - Needle and Hay are some bonkers variable names. I had a tough time grokking all this code.
- If this works consistently then why is there a constructor that lets users pass a custom
getGenericMethodOfConcreteTypeimplementation? If this doesn't work consistently then why have it at all?
There was a problem hiding this comment.
- Wouldn't something like this be equivalent?
AccessTools.Method(needleMethod.Name, needleMethod.GetParameters())Or is there something I'm not seeing?
I have no idea how that handles instances of the generic parameters of the containing type or the method 🤔
2. Needle and Hay are some bonkers variable names. I had a tough time grokking all this code.
Went with the needle and haystack metaphor for searching something, but I guess it's not super clear in this context lol
3. If this works consistently then why is there a constructor that lets users pass a custom
getGenericMethodOfConcreteTypeimplementation? If this doesn't work consistently then why have it at all?
I'm not sure, having the Func is kind of just what I first came up with
|
there you go @ljoonal, I made sure the doc will actually be useful now, too ;) |
| /// <summary> | ||
| /// Represents a generic method invoker that invokes potentially generic methods on generic types. | ||
| /// </summary> | ||
| public sealed class GenericTypeMethodsInvoker |
There was a problem hiding this comment.
Could a short <example><code>, since this seems to be the "entry point" of this utility API? 😜 But yeah at least some docs is better than no docs 👍
Currently still without documentation, but the first goal would be to have other people look over the API.
And add a proper matcher for
GenericTypeMethodsInvoker.GetGenericMethodOfConcreteTypeDefault😂