Batch r nuget: commands #682

KevinRansom · 2019-12-04T10:35:55Z

No description provided.

Microsoft.DotNet.Interactive.CSharp/CSharpKernel.cs

Microsoft.DotNet.Interactive.CSharp/CSharpKernelExtensions.cs

jonsequitur · 2019-12-04T14:31:57Z

Microsoft.DotNet.Interactive.Tests/LanguageKernelTests.cs

            events
                .Should()
-                .ContainSingle(e => e is NuGetPackageAdded);
+                .ContainSingle(e => e is DisplayedValueUpdated && ((DisplayedValueUpdated)e).Value.ToString().Contains("done!"));


This looks like a much weaker assertion than checking for the specific event type.

Immediately below is this assertion, which accurately verifies the event:
The line above verifies that the updated output text is produced.

events.OfType<PackageAdded>() .Should() .BeEquivalentTo(new NugetPackageReference("Microsoft.Extensions.Logging", "2.2.0")); .ContainSingle(e => ((PackageAdded)e).PackageReference.PackageName == "Microsoft.Extensions.Logging" && ((PackageAdded)e).PackageReference.PackageVersion == "2.2.0");

Microsoft.DotNet.Interactive/Commands/LoadExtensionsInDirectory.cs

jonsequitur · 2019-12-04T14:36:07Z

Microsoft.DotNet.Interactive/Commands/NugetRestoreDirective.cs

+
+namespace Microsoft.DotNet.Interactive.Commands
+{
+    public class NugetRestoreDirective : KernelCommandBase


Command naming is typically imperative, e.g. RestoreNugetPackage.

jonsequitur · 2019-12-04T14:37:24Z

Microsoft.DotNet.Interactive/KernelExtensionLoader.cs

            IKernel kernel, 
            KernelInvocationContext context,
-            IEnumerable<FileInfo> additionalDependencies = null)
+            IEnumerable<string> additionalDependencies = null)


I think additionalDependencies should be able to be removed in favor of something easier to understand. Not sure if you've seen an opportunity to do that but don't be worried about breaking existing extension functionality because it needs to be redone anyway.

jonsequitur · 2019-12-04T14:39:07Z

Microsoft.DotNet.Interactive/PackageRestoreContext.cs

+            var packageKey = requestedPackage.PackageKey;

-            if (!string.IsNullOrEmpty(packageName) && _nugetPackageReferences.TryGetValue(requestedPackage, out _))
+            lock (_packageReferences)


Rather than needing a lock, you can use ConcurrentDictionary.

jonsequitur · 2019-12-10T20:57:19Z

Microsoft.DotNet.Interactive/PackageRestoreContext.cs

                                   probingPaths))
                       .ToDictionary(r => r.PackageName, StringComparer.OrdinalIgnoreCase);

+            lock (_lockObject)


lock (_lockObject) [](start = 11, length = 19)

This lock looks unnecessary.

jonsequitur · 2019-12-10T20:59:18Z

Microsoft.DotNet.Interactive/Utility/PackageRestoreResult.cs

+    {
+        public PackageRestoreResult(
+            bool succeeded,
+            IEnumerable<PackageReference> requestedPackages,


IEnumerable requestedPackages, [](start = 12, length = 48)

This is unused.

jonsequitur · 2019-12-10T21:01:15Z

Microsoft.DotNet.Interactive.Tests/LanguageKernelTests.cs

        }

+        [Fact]
+        public async Task PoundRNugetDisallowsEmptyPackageSpecification()


PoundRNugetDisallowsEmptyPackageSpecification [](start = 26, length = 45)

Test naming quibble: snake case please.

jonsequitur · 2019-12-10T21:14:43Z

Microsoft.DotNet.Interactive/Commands/RestoreNugetDirective.cs

+
+namespace Microsoft.DotNet.Interactive.Commands
+{
+    public class RestoreNugetDirective : KernelCommandBase


RestoreNugetDirective [](start = 17, length = 21)

Would RestorePackages capture the intent here? You've removed "Nuget" from other class names, and "Directive" sounds like the direct object here, which isn't accurate.

KevinRansom · 2019-12-11T00:55:40Z

@jonsequitur
It tells me I can't merge the PR. Nor can I resolve the conflicts.

I will resolve them On my PC and push another commit.

jonsequitur · 2019-12-11T20:38:40Z

Microsoft.DotNet.Interactive/Events/PackageAdded.cs

+                throw new System.ArgumentNullException(nameof(addPackage));
+            }
+
+            PackageReference = addPackage.PackageReference;


It's preferable to take in the PackageReference directly here. Then long as the parameter name matches the property name (case-insensitive), the deserialization will work correctly.

jonsequitur · 2019-12-11T20:41:05Z

Microsoft.DotNet.Interactive.Tests/LanguageKernelTests.cs

+                .Last()
+                .Value
+                .Should()
+                .Be("");


Why this change? This doesn't look like the correct behavior.

Hmm, it isn't ...

KevinRansom · 2019-12-12T06:30:13Z

@jonsequitur
GitHub hates me:

jonsequitur reviewed Dec 4, 2019

View reviewed changes

Microsoft.DotNet.Interactive.CSharp/CSharpKernel.cs Outdated Show resolved Hide resolved

jonsequitur reviewed Dec 4, 2019

View reviewed changes

Microsoft.DotNet.Interactive.CSharp/CSharpKernelExtensions.cs Outdated Show resolved Hide resolved

jonsequitur reviewed Dec 4, 2019

View reviewed changes

Microsoft.DotNet.Interactive/Commands/LoadExtensionsInDirectory.cs Outdated Show resolved Hide resolved

jonsequitur reviewed Dec 4, 2019

View reviewed changes

KevinRansom added 2 commits December 5, 2019 13:06

Batch r nuget: commands

c17a572

feedback

d1995e8

KevinRansom force-pushed the batchcommands-merged branch from e625211 to 5191f5c Compare December 6, 2019 00:06

KevinRansom added 2 commits December 6, 2019 12:49

tests and improvements

7ebae94

Rename

32459a0

KevinRansom force-pushed the batchcommands-merged branch from eabeea9 to 32459a0 Compare December 6, 2019 21:15

Temp

1b10a64

jonsequitur reviewed Dec 10, 2019

View reviewed changes

jonsequitur approved these changes Dec 10, 2019

View reviewed changes

jonsequitur reviewed Dec 10, 2019

View reviewed changes

Remove unused SourceCode property from PackageReference

605a2af

merge

27313f8

jonsequitur reviewed Dec 11, 2019

View reviewed changes

KevinRansom and others added 4 commits December 11, 2019 14:36

Snake_name tests

76c5030

serialization

5a3d9d4

System.Text.Json

d5e27f5

commandenvelope

61f109e

jonsequitur merged commit d0fbb15 into dotnet:master Dec 12, 2019

Batch r nuget: commands #682

Batch r nuget: commands #682

Uh oh!

Conversation

KevinRansom commented Dec 4, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevinRansom Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevinRansom commented Dec 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevinRansom commented Dec 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

KevinRansom Dec 4, 2019 •

edited

Loading