Fixes to add package command #1086
Conversation
| @@ -24,6 +24,7 @@ namespace NuGet.XPlat.FuncTest | |||
| public class XPlatAddPkgTests | |||
| { | |||
| private static readonly string projectName = "test_project_addpkg"; | |||
| private static MSBuildAPIUtility msBuild = new MSBuildAPIUtility(new TestCommandOutputLogger()); | |||
There was a problem hiding this comment.
I would avoid sharing this between tests, you could make this a property that returns a new instance each time if needed though.
| private const string UPDATE_OPERATION = "Update"; | ||
| private const string REMOVE_OPERATION = "Remove"; | ||
|
|
||
| public ILogger Logger { get; } |
There was a problem hiding this comment.
I don't see a lot of state in this class beyond the logger. Maybe this should be a static class instead and the methods could take the logger as needed.
There was a problem hiding this comment.
discussed offline. Instantiable class allows mocking and easier unit testing.
| .Select(f => NuGetFramework.Parse(f))); | ||
|
|
||
| restoreGraphs = restoreGraphs | ||
| .Where(r => userSpecifiedFrameworks.Contains(r.Framework)); |
There was a problem hiding this comment.
What if the user accidentally specified a framework that doesn't exist in the project? a error message for that scenario might help, looks like it would just go on and possibly fail with an unrelated error message
| .Select(p => p) | ||
| .Where(p => p.Key.Name.Equals(packageReferenceArgs.PackageDependency.Id, StringComparison.OrdinalIgnoreCase)); | ||
|
|
||
| if (matchingPackageEntries.Count() > 0) |
There was a problem hiding this comment.
nit: matchingPackageEntries.Any()
| } | ||
| } | ||
|
|
||
| private NuGetVersion privateGetPackageVersionFromRestoreResult(RestoreResultPair restorePreviewResult, |
There was a problem hiding this comment.
private static NuGetVersion GetPackageVersionFromRestoreResult
it is a good practice to make methods static if possible, here there are no class fields or properties being used that I see
method names should be Camel cased per the C# style guidelines
| // Get the package version from the graph | ||
| var resolvedVersion = privateGetPackageVersionFromRestoreResult(restorePreviewResult, packageReferenceArgs); | ||
|
|
||
| if (resolvedVersion != null) |
There was a problem hiding this comment.
I don't see where null is being handled. If finding the highest version comes back as null it looks like this just keeps going
| VersionRange.Parse(resolvedVersion.ToString())); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
It might make the code more clear to just return the version here, it would be easier to handle errors as needed from a higher level.
| @@ -398,4 +398,29 @@ For more information, visit http://docs.nuget.org/docs/reference/command-line-re | |||
| <data name="Error_NoDgSpec" xml:space="preserve"> | |||
| <value>None or invalid DgSpec was passed to NuGet add package command.</value> | |||
| </data> | |||
| <data name="Error_AddPkgErrorStringForImportedEdit" xml:space="preserve"> | |||
| <value>Item : {0} for {1} in Imported file : {2}</value> | |||
There was a problem hiding this comment.
Is this colon delimited format used anywhere else? I would expect this to feel very similar to the add project command if possible.
| { | ||
| //Update the packagedependency with the new version | ||
| packageReferenceArgs.PackageDependency = new PackageDependency(packageReferenceArgs.PackageDependency.Id, | ||
| VersionRange.Parse(resolvedVersion.ToString())); |
There was a problem hiding this comment.
There's no need to parse this as a string. Just pass the resolvedVersion as the min version.
mishra14
force-pushed
the
dev-anmishr-addrefpatch
branch
from
fab0284
to
bca4328
Compare
Finishes work for : NuGet/Home#3751
Changes -
@emgarten @rrelyea