Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove deleted argument to VisitSubkernelsAndSelf #182

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

markples
Copy link
Contributor

In dotnet/interactive#3563, the VisitSubkernelsAndSelf method (in src/Microsoft.DotNet.Interactive/KernelExtensions.cs) was updated to remove the recursive argument, which breaks XPlot:

     public static void VisitSubkernelsAndSelf(
         this Kernel kernel,
-        Action<Kernel> onVisit,
-        bool recursive = false)
+        Action<Kernel> onVisit)
     {
         if (kernel is null)

This should fix the binding, though I haven't built or tested anything yet.

Two possible issues:

  • This should still be able to build against older versions of Microsoft.DotNet.Interactive because they had a default value for the deleted argument, but binaries will need to run against compatible versions because the signature of the call is explicit in the IL. Specifically, new XPlot won't work with old Interactive.
  • New VisitSubkernelsAndSelf is only non-recursive, so this is a behavior change from before (which passed true).

@markples
Copy link
Contributor Author

It looks like this might need changes similar to #172.

@jonsequitur
Copy link

You can also remove the binary dependency on .NET Interactive entirely using convention-based formatting. plotly/Plotly.NET#476 (comment)

@cartermp cartermp merged commit 5735829 into fslaborg:main Nov 14, 2024
2 checks passed
@cartermp
Copy link
Collaborator

@markples @jonsequitur I'll see if I can give building and updating this thing a whirl this afternoon. It was painful to update all the things late last year in part due to the build dependencies, but it's likely different now.

@cartermp
Copy link
Collaborator

Okay, this is now in latest main alongside an update to packages, tfms, etc. I now need to remember how to log into nuget so I can actually update packages. Ugh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants