#176: Adding support for aggregated/interhited interfaces #523
Conversation
|
|
|
Please add some integration tests with the http mocks as well. |
…mplateInformation object
…ods by it's interfaces
|
I've added an integration test and found an issue which I've fixed already. Sorry for not coming up with this in the first place. Is there anything else which is needed from your perspective? |
| return ret; | ||
| } | ||
|
|
||
| private static void AddInheritedMethods(TemplateInformation ret) | ||
| { | ||
| foreach (var c in ret.ClassList.Where(c => c.BaseClassNames != null && c.BaseClassNames.Any())) |
There was a problem hiding this comment.
Does this logic handle two interfaces with the same name but in different namespaces?
|
Looks good but please add a few integration tests for the same interface name but in different namespaces. One of them would need to be namespace qualified to work. Another test should use a using alias for the interface name. That should help ensure the code gen is correct and covers the bases. |
|
|
||
| private void AddInterfaceHttpMethods(Type interfaceType, Dictionary<string, List<RestMethodInfo>> methods) | ||
| { | ||
| foreach (var methodInfo in interfaceType.GetMethods()) { |
There was a problem hiding this comment.
Please fix this formatting. Braces should be on the next line.
|
Also, what if the interfaces have the same method name but different return types? I think that the code gen should be updated to do explicit interface implementations as that's probably the only way to ensure correctness? There should be a test for that scenario too (same method name with same parameters on two diff interfaces but different return types) |
|
can we merge this? |
|
Not yet because the questions I have had not been answered yet. It's not clear yet they won't cause issues. If someone wants to add tests to cover those scenarios and show that it works (or fixes it if it doesn't), I'll be happy to merge. |
|
thank you for your fast response, how can i build it to test it, i have a scenario where i need this feature |
|
It should be directly buildable in Visual Studio 2017. |
|
couldn't manage to do it, removed the inheritance from my code. hopefully this scenario is supported soon. Switched from restease to refit because dynamic code generation isn't supported on iOS. Thank you for your support. |
|
This looks like a very useful feature, is there an ETA on when this will be merged, or is there a preview package that includes this code that we can try out? 😄 |
|
@Sergio0694 this PR needs additional validation tests and potential fixes as I describe here: #523 (comment). Someone can feel free to pick it up and finish it. Happy to merge once it's tested and complete. |
|
Hey @Iss0 - are you still working on this? This pull request looks great so far! @onovotny I have a question, why isn't it okay if the build just fails in case someone tries to inherit from two interfaces that have two methods with the same name and different signature? Thanks! |
|
Hi @Sergio0694! Actually I totally forgot about this. I can see @onovotny points and I'm planning to work on it soon. Hope to find the time in next week. |
|
Hi @Iss0 - thank you so much for your quick reply and for your help! Another great upside to having inherited interfaces is that it'll be possible to write extension methods for the base interfaces, to be able to perform some custom post processing of the data returned from the APIs in use. Right now you either have to setup a custom JSON converter every time (for each interface) or to put the additional code somewhere else, and have it call the method in the interface, and then process the results. Instead with this it could all be hidden in an extension method that worked on all the derived interfaces. I guess this could be improved even more once C# 8.0 comes out, by just integrating the additional code directly in the base interfaces using base interface implementations. This would make this new Refit feature even more powerful by then 🎉 |
|
Hey @Iss0 - sorry to bother, any updates on this? EDIT: not sure if you're still interested in this, but I've proceeded to fork from your own fork and implement the requested explicit interface implementation feature, it's all in this PR: #633. |
Like explained in issue #176 it was not possible to do something like the following
I created a simple solution which makes it possible to have inheritance like in the example.