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

Fix for loaded script files ignoring #nowarn #1239

Merged
merged 2 commits into from
Jun 6, 2016

Conversation

taylorwood
Copy link
Contributor

I took a stab at fixing issue #1126. It didn't seem that the loaded script closure's #nowarn directives were being applied in fsi, like they are in fsc here: https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/fsc.fs#L241

I wasn't sure where a test might belong so I stuck one in a convenient place. Here's what it looks like w/o the fix in place:
fail

@@ -1072,6 +1072,8 @@ type internal FsiDynamicCompiler
else fsiConsoleOutput.uprintnf " %s %s" (FSIstrings.SR.fsiLoadingFilesPrefixText()) sourceFile)
fsiConsoleOutput.uprintfn "]"

closure.NoWarns |> Seq.map (fun (n,ms) -> ms |> Seq.map (fun m -> m,n)) |> Seq.concat |> Seq.iter tcConfigB.TurnWarningOff
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like good. Had to go look up a few things to check, but it looks right.

@dsyme
Copy link
Contributor

dsyme commented Jun 3, 2016

The fix is good and the test appropriate.

@@ -44,4 +44,6 @@ echo Test 16=================================================
"%FSC%" ProjectDriver.fsx --nologo
ProjectDriver.exe
del ProjectDriver.exe
echo Test 17=================================================
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@enricosada has a pending PR that deletes these old test .bat files, but I didn't know about it until after I submitted this and I suppose this might cause a little merge conflict

Copy link
Contributor

Choose a reason for hiding this comment

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

np @taylorwood , i'll rebase, dont care about my pr, you already update the tests/fsharp/core/tests_core.fs so it's ok

@KevinRansom KevinRansom merged commit d6a2426 into dotnet:master Jun 6, 2016
@KevinRansom
Copy link
Member

@taylorwood
Thank you for making this change

Kevin

@taylorwood taylorwood deleted the load-nowarn branch June 6, 2016 21:37
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.

5 participants