-
Notifications
You must be signed in to change notification settings - Fork 563
Add xplot support #365
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
Add xplot support #365
Conversation
| var kernel = CreateKernel(); | ||
| kernel.UseDefaultExtensions(); | ||
|
|
||
| await kernel.SendAsync(new SubmitCode("#r nuget:XPlot.Plotly")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for now this shouldn't be needed (although we'll need to modify the ScriptOptions in CSharpKernel). Like PocketView and the display helper, it should be available by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case things will work in c# out of the box and not for fsharp unless we explicitly add that dependency for f# as well. Do we need a list of references "somewhere" and those should be added to both f# and c# by default ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brettfo Thoughts?
|
|
||
| html.AppendLine(@"});"); | ||
|
|
||
| html.Append(chartHtml.AsSpan().Slice(scriptEnd).ToString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use the same StringBuilder overloads as you have above - (string, start, length), and not create a span and call ToString on it, since that creates and copies unnecessary memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even simpler is to pass in the TextWriter from the Formatter.Register call on line 15 and write to it directly. No StringBuilder needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even better. In that case, I'd suggest making this project target netcoreapp2.1 or higher (or netstandard2.1), and then take advantage of the new ReadOnlySpan overload - https://docs.microsoft.com/en-us/dotnet/api/system.io.textwriter.write?view=netcore-2.1#System_IO_TextWriter_Write_System_ReadOnlySpan_System_Char__.
I'm sure it won't make a dramatic different performance wise, but why make the computer do unnecessary work when it doesn't have to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of parsing strings, I have modified the code to use the HTMLAgilityPack
XPlot.DotNet.Interactive.KernelExtensions/XPlotKernelExtension.cs
Outdated
Show resolved
Hide resolved
XPlot.DotNet.Interactive.KernelExtensions/XPlotKernelExtension.cs
Outdated
Show resolved
Hide resolved
| { | ||
| public partial class XplotKernelExtensionTests : CSharpKernelTestBase | ||
| { | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra blank line
| public Task OnLoadAsync(IKernel kernel) | ||
| { | ||
| Formatter<PlotlyChart>.Register((chart, writer) => | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird indenting
This PR adds a KernelExtension which is loaded by default for now. On loading this extension adds a formatter for the "PlotlyChart". This formatter produces a javascript which looks something like this: