-
Notifications
You must be signed in to change notification settings - Fork 177
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.
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
Conversation
@filipw Left a comment here, but maybe it should be looked at in another PR ? |
which comment though? 😁 |
@@ -129,6 +108,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; |
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.
@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?
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.
@filipw Forgot to submit the review. That's why you did not see the comment :)
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.
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
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.
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 😄
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.
@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);
}
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.
@filipw I'll add the tests if you are short on time ? 👍
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.
yep, good idea - I will add them
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 were right, it didn't actually work 😂
see this changeset, should be fine now 75f8934
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.
Awesome 🥇
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'd say we are good to go for 0.16.0
We support running a script + dropping you into the REPL using
dotnet script foo.csx -i
start up arguments.This PR aligns how the output (errors, return values) are dumped into the console when running a REPL with seed vs just running a script.
It also ensures that in such "seed" cases, the arguments are correctly surfaced to the script.