(GH-17) Use dynamic loading of addins #127
Conversation
There was a problem hiding this comment.
What bothers me about this approach is, that it requires to basically redefine the whole API of every addin (once used for everything) using reflection. Which will be a significant effort (there are currently over 75 aliases and much more public classes). Therefore I don't see this as the approach which works for every case. Another possibility would be to write the code just as a string which is then executed at runtime.
I think in the end we need to find a good balance between which addins are added always (e.g. Cake Issues core addins), which are loaded dynamic but executed as string and aliases and API is not available to scripts using Cake.Issues.Recipe and which of the API is made available to users.
| @@ -113,16 +115,31 @@ IssuesBuildTasks.CreateFullIssuesReportTask = Task("Create-FullIssuesReport") | |||
| IssuesParameters.OutputDirectory.CombineWithFilePath(reportFileName); | |||
| EnsureDirectoryExists(IssuesParameters.OutputDirectory); | |||
|
|
|||
| IIssueReportFormat issueFormat = null; | |||
|
|
|||
| if (!Context.Environment.Runtime.IsCoreClr) | |||
There was a problem hiding this comment.
Loading of addins should mainly be triggered by parameters. Meaning that if any parameter is set which requires the reporting addin it should be loaded. In case of the reporting addin which is currently not compatible with .NET Core we can decide what to do in this case (IMHO it should be an error, since the user says he want's report and the script is not capable of creating one).
There was a problem hiding this comment.
Well, I didn't want to implement any changes to the options class for this PR, which is why I just added a check if we are running on .NET Core or not.
But I do agree that it should be triggered by parameters.
Only if you need to use everything, basically the intention is to just define what you need instead of the whole API.
Well, I could definitely try to re-implement a more generic approach with this in thought. |
AdmiringWorm
force-pushed
the
load-with-reflection
branch
from
543961e
to
e625449
Compare
|
@pascalberger I changed the logic up a bit to allow for more loosely coupled reflection (like passing in string as a method name), instead of redefining the API (not yet complete, but it is a start I believe). Hopefully this is a bit closer to what you had in mind. |
AdmiringWorm
force-pushed
the
load-with-reflection
branch
2 times, most recently
from
da0b54f
to
7156d2f
Compare
pascalberger
force-pushed
the
load-with-reflection
branch
from
7156d2f
to
cbf6356
Compare
This pull requests starts the first steps of allowing addins to be loaded dynamically,
I am starting with the Generic Reporter addin as this is the one addin not compatible with .NET Core.
The dynamic loading is done entirely by using reflection after calling a temporary cake script to restore the addin as a tool, and then enumerating throug all of the declared types in the assembly to find the ones with want.
All of this is very much in progress, but would appreciate feedback of what is currently done.
Tested on Windows and Linux using both Cake .NET and Cake .NET Core