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

add simple FSharpKernel wrapper around fsi.exe #357

Merged
merged 9 commits into from Aug 22, 2019

Conversation

brettfo
Copy link
Member

@brettfo brettfo commented Aug 5, 2019

This is more of a proof-of-concept until we decide how we want to properly refactor fsi.

Since we're forced to scrape stdin/stdout, all returned values are forced to text/plain (hence no formatter support because everything is System.String); this will change once we have a proper kernel.

Includes moving most of CSharpKernelTestBase.cs into a common KernelTestBase.cs.

public void Dispose()
{
_disposables?.Dispose();
return new CSharpKernel();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some changes in flight here as well and I'm leaning toward always using the same default set of extensions here as we use in the app at runtime. There's really only one supported configuration at the moment so we should test that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently I think the .UseDefaultRendering() extension should stay only in CSharpKernelTestBase.cs because the F# kernel doesn't support any of that yet since it's really just a thin wrapper/scraper around fsi.

The other alternative is to broaden the extension methods to switch on specific types like so:

public static KernelBase UseDefaultRendering(this KernelBase kernel)
{
    switch (kernel)
    {
        case CSharpKernel csharpKernel:
            csharpKernel.UseDefaultRendering();
            break;
        case FSharpKernel fsharpKernel:
            // noop
            break;
    }

    return kernel;
}

public static KernelBase UseDefaultRendering(this CSharpKernel kernel)
{
    // do C#-specific stuff
}

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than change the type that UseDefaultRendering accepts, maybe we should add an overload for FSharpKernel. The implication of the proposed method is that it should work for all kernel types, which isn't really possible since we can't know all of the possible implementations of KernelBase.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added void SetDefaultRendering() to IKernel so C# can implement it and F#'s is currently a noop.

Also updated to reflect your recent changes to master.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that belongs on IKernel. Rendering is a fairly separate concern.

@brettfo
Copy link
Member Author

brettfo commented Aug 5, 2019

image

@@ -16,5 +16,7 @@ public interface IKernel : IDisposable
IObservable<IKernelEvent> KernelEvents { get; }

Task<IKernelCommandResult> SendAsync(IKernelCommand command, CancellationToken cancellationToken);

void SetDefaultRendering();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no generalization of default rendering that makes sense across all possible kernels, and the concept may not be something we even want to commit to for the two existing kernels. Let's keep the implementations specific to the cases where we understand what this means and keep the interface simpler for now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then move this back to be specific to the C# kernel? That's where we originally started which seems at odds with how I interpreted this comment, specifically, I read this:

maybe we should add an overload for FSharpKernel.

As

move this method to KernelBase

But to properly support CompositeKernel, the method needed to be on IKernel, instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. I meant that we should have two methods, one for CSharpKernel and one for FSharpKernel. Their implementations are different and I don't know if this method will make sense for all kernels.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added commit 03e8866, let me know what you think.

@brettfo
Copy link
Member Author

brettfo commented Aug 17, 2019

Rebased to handle recent changes in master.

@brettfo
Copy link
Member Author

brettfo commented Aug 22, 2019

Ping @jonsequitur for review.

Copy link
Member

@colombod colombod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@brettfo brettfo merged commit 88b5134 into dotnet:master Aug 22, 2019
@brettfo brettfo deleted the fsharp-kernel branch August 22, 2019 20:31
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.

None yet

3 participants