Implement new API for finding standard extensions #1518
Conversation
There was a problem hiding this comment.
@CharliePoole fundamentally code looks good.
A few small remarks/opinions.
| /// <param name="hostAssembly">An assembly that supports extensions.</param> | ||
| public void FindStandardExtensions(Assembly hostAssembly) | ||
| { | ||
| bool isChocolateyPackage = System.IO.File.Exists(Path.Combine(hostAssembly.Location, "VERIFICATION.txt")); |
There was a problem hiding this comment.
Should this use _fileSystem.GetDirectory
There was a problem hiding this comment.
I did that for the FindExtensions method, but in this case it seemed unnecessarily complicated. That is, I would convert a path to an IDirectory, get an IFile from that and then convert them back to a path. But since the calling assembly must always exist, there's always a real path to start with.
There was a problem hiding this comment.
Would that argument not also cover _startDir which comes from the same host assembly?
There was a problem hiding this comment.
I see your point. But of course startDir is defined in a different method and reused multiple times, so it seems worth the trouble. Probably, it would be better to combine those two methods and just get startDir once. I'll add a TODO to my local copy to remind me to do that next time I'm in it.
| ? new[] { "nunit-extension-*/**/tools/", "nunit-extension-*/**/tools/*/" } | ||
| : new[] { "NUnit.Extension.*/**/tools/", "NUnit.Extension.*/**/tools/*/" }; | ||
|
|
||
| FindExtensionAssemblies(hostAssembly, extensionPatterns); |
There was a problem hiding this comment.
Personally I prefer methods returning the set of found assemblies instead of setting a field.
But the approach is pre-existing.
| ProcessAddinsFiles(initialDirectory, false); | ||
| } | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
Should we not use <inheritdoc /> to prevent risking interface and implementation documentation getting out of sync?
There was a problem hiding this comment.
In my own code I have always avoided it but I know others use it. For me it's a question of compatibility. If somebody wants to use a different compiler or ide, do you know if will <inhreitdoc> always work? In any case, if we decided to always use it, then I'd do one big PR rather than changing things one at a time. What do you think?
There was a problem hiding this comment.
The compiler does the copying to the XML file.
See inheritdoc
There was a problem hiding this comment.
The Microsoft compilers do. But maybe I'm beating a dead horse. Currently, I can't find any viable alternatives to the MS compiler, such as we used to have a long time ago. And of course we're using it to build the engine anyway.
I don't like to combine unrelated changes in the same PR, so I'll create an issue.
|
@manfred-brands I implemented one of your three suggestions and merged so as to be able to work some issues that depend on this change. Feel free to comment further, particularly wrt |
Fixes #1504 Replaces PR #1506 Part of #488
@manfred-brands I'd appreciate your re-review based on the changes made since you reviewed the original PR (second commit)