Skip to content

align the behavior between REPL, REPL seeded with a script and script #187

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Nov 27, 2017
Merged

align the behavior between REPL, REPL seeded with a script and script #187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8ecac31
align the behavior between REPL and REPL seeded with a script
filipw Nov 24, 2017
2897965
seed tests
filipw Nov 24, 2017
5fb8904
Merge branch 'master' into feature/align-seeding
filipw Nov 27, 2017
75f8934
ensure nuget #load and #r work on first line of REPL
filipw Nov 27, 2017
858435f
Merge branch 'feature/align-seeding' of https://github.com/filipw/dot…
filipw Nov 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 35 additions & 26 deletions src/Dotnet.Script.Core/Interactive/InteractiveRunner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,13 @@ public virtual async Task RunLoop(bool debugMode)

public virtual async Task RunLoopWithSeed(bool debugMode, ScriptContext scriptContext)
{
await RunFirstScript(scriptContext);
await HandleScriptErrors(async () => await RunFirstScript(scriptContext));
await RunLoop(debugMode);
}

protected virtual async Task Execute(string input, bool debugMode)
{
try
await HandleScriptErrors(async () =>
{
if (_scriptState == null)
{
Expand All @@ -72,8 +72,7 @@ protected virtual async Task Execute(string input, bool debugMode)
}
else
{
var lineRuntimeDependencies =
ScriptCompiler.RuntimeDependencyResolver.GetDependenciesFromCode(CurrentDirectory, input).ToArray();
var lineRuntimeDependencies = ScriptCompiler.RuntimeDependencyResolver.GetDependenciesFromCode(CurrentDirectory, input).ToArray();
var lineDependencies = lineRuntimeDependencies.SelectMany(rtd => rtd.Assemblies).Distinct();

var scriptMap = lineRuntimeDependencies.ToDictionary(rdt => rdt.Name, rdt => rdt.Scripts);
Expand All @@ -92,28 +91,7 @@ protected virtual async Task Execute(string input, bool debugMode)

_scriptState = await _scriptState.ContinueWithAsync(input, _scriptOptions, ex => true);
}

if (_scriptState?.Exception != null)
{
Console.WritePrettyError(CSharpObjectFormatter.Instance.FormatException(_scriptState.Exception));
}

if (_scriptState?.ReturnValue != null)
{
_globals.Print(_scriptState.ReturnValue);
}
}
catch (CompilationErrorException e)
{
foreach (var diagnostic in e.Diagnostics)
{
Console.WritePrettyError(diagnostic.ToString());
}
}
catch (Exception e)
{
Console.WritePrettyError(CSharpObjectFormatter.Instance.FormatException(e));
}
});
}

public virtual void Reset()
Expand All @@ -129,6 +107,9 @@ public virtual void Exit()

private async Task RunFirstScript(ScriptContext scriptContext)
{
foreach (var arg in scriptContext.Args)
_globals.Args.Add(arg);

var compilationContext = ScriptCompiler.CreateCompilationContext<object, InteractiveScriptGlobals>(scriptContext);
_scriptState = await compilationContext.Script.RunAsync(_globals, ex => true).ConfigureAwait(false);
_scriptOptions = compilationContext.ScriptOptions;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@filipw Do we handle the case where #load "nuget:scriptpackage,123" or #r "nuget:package,1,2,3" is the first line in interactive mode?

Copy link
Collaborator

@seesharper seesharper Nov 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@filipw Forgot to submit the review. That's why you did not see the comment :)

Copy link
Member Author

Choose a reason for hiding this comment

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

this is all handled when going through CreateCompilationContext, isn't it? here https://github.com/filipw/dotnet-script/blob/9045fe6745313ad412d2d54c3c3d0f3e093e25eb/src/Dotnet.Script.Core/ScriptCompiler.cs#L95

it uses the same ScriptCompiler as regular script runner would use

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, right. You are probably correct. I'm thinking maybe adding two new tests that verifies this.

one with #r "nuget:package,1.2.3 as the first line and one with #load "nuget:scriptpackage, 1.2.2" and the first line. Just to be sure 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

@filipw Something like

 [Fact]
        public async Task NugetPackageReferenceAsTheFirstLine()
        {
            var commands = new[]
            {
                @"#r ""nuget: Automapper, 6.1.1""",
                "using AutoMapper;",
                "typeof(MapperConfiguration)",
                "#exit"
            };

            var ctx = GetRunner(commands);
            await ctx.Runner.RunLoop(false);

            var result = ctx.Console.Out.ToString();
            Assert.Contains("[AutoMapper.MapperConfiguration]", result);
        }

Copy link
Collaborator

Choose a reason for hiding this comment

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

@filipw I'll add the tests if you are short on time ? 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, good idea - I will add them

Copy link
Member Author

Choose a reason for hiding this comment

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

you were right, it didn't actually work 😂
see this changeset, should be fine now 75f8934

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome 🥇

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say we are good to go for 0.16.0

Expand Down Expand Up @@ -156,5 +137,33 @@ private string ReadInput()

return input.ToString();
}

private async Task HandleScriptErrors(Func<Task> doWork)
{
try
{
await doWork();
if (_scriptState?.Exception != null)
{
Console.WritePrettyError(CSharpObjectFormatter.Instance.FormatException(_scriptState.Exception));
}

if (_scriptState?.ReturnValue != null)
{
_globals.Print(_scriptState.ReturnValue);
}
}
catch (CompilationErrorException e)
{
foreach (var diagnostic in e.Diagnostics)
{
Console.WritePrettyError(diagnostic.ToString());
}
}
catch (Exception e)
{
Console.WritePrettyError(CSharpObjectFormatter.Instance.FormatException(e));
}
}
}
}
8 changes: 4 additions & 4 deletions src/Dotnet.Script.Core/ScriptCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,11 @@ public virtual ScriptCompilationContext<TReturn> CreateCompilationContext<TRetur
var platformIdentifier = RuntimeHelper.GetPlatformIdentifier();
Logger.Verbose($"Current runtime is '{platformIdentifier}'.");

IList<RuntimeDependency> runtimeDependencies =
RuntimeDependencyResolver.GetDependencies(context.WorkingDirectory).ToList();
var runtimeDependencies = context.FilePath != null
? RuntimeDependencyResolver.GetDependencies(context.WorkingDirectory)
: RuntimeDependencyResolver.GetDependenciesFromCode(context.WorkingDirectory, context.Code.ToString());


var opts = CreateScriptOptions(context, runtimeDependencies);
var opts = CreateScriptOptions(context, runtimeDependencies.ToList());

var runtimeId = RuntimeHelper.GetRuntimeIdentifier();
var inheritedAssemblyNames = DependencyContext.Default.GetRuntimeAssemblyNames(runtimeId).Where(x =>
Expand Down
1 change: 0 additions & 1 deletion src/Dotnet.Script.DependencyModel/Runtime/RuntimeDependencyResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ public IEnumerable<RuntimeDependency> GetDependenciesFromCode(string targetDirec
return GetDependenciesInternal(pathToProjectFile);
}


public IEnumerable<RuntimeDependency> GetDependencies(string targetDirectory)
{
var pathToProjectFile = _scriptProjectProvider.CreateProject(targetDirectory, "netcoreapp2.0", true);
Expand Down
73 changes: 72 additions & 1 deletion src/Dotnet.Script.Tests/InteractiveRunnerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.IO;
using System.Text;
using System;
using Microsoft.CodeAnalysis.Text;

namespace Dotnet.Script.Tests
{
Expand Down Expand Up @@ -66,7 +67,7 @@ public async Task SimpleOutput()
}

[Fact]
public async Task Exception()
public async Task RuntimeException()
{
var commands = new[]
{
Expand All @@ -81,6 +82,41 @@ public async Task Exception()
Assert.Contains("(1,1): error CS0103: The name 'foo' does not exist in the current context", result);
}

[Fact]
public async Task ValueFromSeededFile()
{
var commands = new[]
{
"x+x",
"#exit"
};

var ctx = GetRunner(commands);
await ctx.Runner.RunLoopWithSeed(false, new ScriptContext(SourceText.From(@"var x = 1;"), Directory.GetCurrentDirectory(), new string[0]));

var result = ctx.Console.Out.ToString();
Assert.Contains("2", result);
}

[Fact]
public async Task RuntimeExceptionFromSeededFile()
{
var commands = new[]
{
"var x = 1;",
"x+x",
"#exit"
};

var ctx = GetRunner(commands);
await ctx.Runner.RunLoopWithSeed(false, new ScriptContext(SourceText.From(@"throw new Exception(""die!"");"), Directory.GetCurrentDirectory(), new string[0]));

var errorResult = ctx.Console.Error.ToString();
var result = ctx.Console.Out.ToString();
Assert.Contains("2", result);
Assert.Contains("die!", errorResult);
}

[Fact]
public async Task Multiline()
{
Expand Down Expand Up @@ -212,5 +248,40 @@ public async Task ResetCommand()
var errResult = ctx.Console.Error.ToString();
Assert.Contains("error CS0103: The name 'x' does not exist in the current context", errResult);
}

[Fact]
public async Task NugetPackageReferenceAsTheFirstLine()
{
var commands = new[]
{
@"#r ""nuget: Automapper, 6.1.1""",
"using AutoMapper;",
"typeof(MapperConfiguration)",
"#exit"
};

var ctx = GetRunner(commands);
await ctx.Runner.RunLoop(false);

var result = ctx.Console.Out.ToString();
Assert.Contains("[AutoMapper.MapperConfiguration]", result);
}

[Fact]
public async Task ScriptPackageReferenceAsTheFirstLine()
{
var commands = new[]
{
@"#load ""nuget: simple-targets-csx, 6.0.0""",
"using static SimpleTargets;",
"typeof(TargetDictionary)",
"#exit"
};

var ctx = GetRunner(commands);
await ctx.Runner.RunLoop(false);
var result = ctx.Console.Out.ToString();
Assert.Contains("[Submission#0+SimpleTargets+TargetDictionary]", result);
}
}
}