Add ability to link multiple referenced packages at once #27
Conversation
…e name or only root dir.
… not send any package then it unlinks all.
… could be selected to be linked to. Changed reporting level when using solution wide linking and unlinking.
Now handles correctly cases where package is already linked and mode is not single to single.
…ne of them only contains MSBuild targets/tasks.
There was a problem hiding this comment.
Hi @danieljbfaustino, this is a great addition to the tool, thanks again! I want it merged.
Yet this isn't a trivial feature, and it has a few tricky aspects, which we have to sort out first - please see my comments below.
I also suggested a couple of styling improvements (I am really sorry for being meticulous - I'm guarding the code to be readable and clean).
Last but not least, we have to add tests that cover the new functionality before we can merge it. The relevant files are LinkTests.cs and UnlinkTests.cs, these are acceptance tests that actually invoke the tool. I'm going to write a document on these, but I can't promise when. Let me know if you need guidance.
I think this is a great feature that pushes the tool to the next level. But if the effort to complete it seems overwhelming, and maybe you didn't plan to invest that much, just let me know - I will continue from where you stop. This PR is a good first step.
| @@ -2,6 +2,6 @@ namespace NuLink.Cli | |||
| { | |||
| public interface INuLinkCommand | |||
| { | |||
| int Execute(NuLinkCommandOptions options); | |||
| void Execute(NuLinkCommandOptions options); | |||
There was a problem hiding this comment.
The idea behind the int return value is to return an exit code, which can be useful for scripting. I guess int becomes tricky because of a multi-package operation, which can result in a mix of successes and failures. In such a case, I'd prefer to define the overall success and failure.
This is what I suggest:
- For
linkcommand, the "package already linked" situation, when the link target is the desired location, should be a success rather than an error (print a success message, do nothing). - For
unlinkcommand, if the package is not linked, it's a success and not an error (print a success message, do nothing). - Continue to handle all other errors
- Let the user choose one of two modes:
- Fail fast (default): stop on the first error and exit with an error code
- Ignore errors: if an
--ignore-errorsflag is specified, we run all packages, report all errors as warnings, and always exit with code 0.
(as for implementation, see comment below about exceptions)
| var allPackages = GetAllPackages(options); | ||
| var localProjectPath = options.LocalProjectPath; | ||
|
|
||
| if (options.Mode != NuLinkCommandOptions.LinkMode.AllToAll) |
There was a problem hiding this comment.
A styling hint: in order to keep functions short, I'd extract "then" and "else" into two nested functions.
| } | ||
|
|
||
| void ValidateOperation() | ||
| bool ValidateOperation() |
There was a problem hiding this comment.
TL;DR: let's keep throwing exceptions rather than returning a bool.
I was thinking quite a bit about this one. If we want to support both "ignore errors" and "fail fast" modes, the code can become complicated. I mean that using exception results in simpler code. On the other hand, we have "do not use exceptions to steer logic".
In the bottom line, I think that throwing exceptions from LinkPackage() is OK, because LinkPackage should either perform its responsibility or throw. We can implement both "fail fast" and "ignore" by catching at the caller (inside the foreach loop). When in "ignore" mode, report a warning. When in "fail fast" mode, re-throw.
|
|
||
| if (options.Mode == NuLinkCommandOptions.LinkMode.SingleToAll) | ||
| { | ||
| localProjectPath = GetAllProjects(options.RootDirectory). |
There was a problem hiding this comment.
I'd avoid reassigning a local variable because it reduces readability.
Instead, you can introduce a new variable named effectiveLocalProjectPath and use the ? : operator to assign it either localProjectPath or the result of lookup with GetAllProjects().
| } | ||
|
|
||
| if (status.IsLibFolderLinked) | ||
| { | ||
| throw new Exception($"Error: Package {requestedPackage.PackageId} is already linked to {status.LibFolderLinkTargetPath}"); | ||
| _ui.Report(singleMode ? VerbosityLevel.Error : VerbosityLevel.Low, () => |
There was a problem hiding this comment.
As I wrote above, this should be an error only if the link target path differs from the requested one. Maybe this check should be performed earlier.
| { | ||
| throw new Exception($"Error: Cannot unlink package {options.PackageId}: 'lib' folder not found, expected {requestedPackage.LibFolderPath}"); | ||
| _ui.Report(allPackages ? VerbosityLevel.Low : VerbosityLevel.Error, () => |
There was a problem hiding this comment.
Let's make it always a success.
Fixes #6
Tool can now be run with:
nulink link -r [repos_root_dir]- if run on the solution's (.sln) directory, will link all referenced packages in all projects within the solution. If run on the project's (.csproj) directory, will link all referenced packages in such project. The user only needs to provide the directory where all repositories are located to the-roption.nulink unlink- if run without options, it will unlink all packages from solution/project, depending on where command is ran.