-
Notifications
You must be signed in to change notification settings - Fork 588
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
Fsi defines #2260
Fsi defines #2260
Conversation
src/app/Fake.DotNet.Fsi/Fsi.fs
Outdated
@@ -75,8 +75,10 @@ type FsiParams = { | |||
(* - LANGUAGE - *) | |||
/// Generate overflow checks | |||
Checked: bool option | |||
/// Define a conditional compilation symbols | |||
/// Define a conditional compilation symbol |
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.
Might be possible to mark this record member Obsolete with a suggestion to use Definitions
?
Though now that I think about it isn't there something about record member with obsolete triggering every time the record is extended using with
expressions?
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.
Yes indeed, I think we can't do that. But we can adjust documentation.
Sad that this slipped through in the process of moving to fake 5 ;)
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've added to the comment to suggest using the new property.
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.
Looks good, please feel free to adjust the comment and answer the question regarding the working directory. But otherwise exactly how it should be done, thanks!
@@ -279,7 +285,7 @@ module internal ExternalFsi = | |||
{ info.WithEnvironmentVariables defaultEnvironmentVars with | |||
FileName = fsiExe | |||
Arguments = args | |||
WorkingDirectory = "" | |||
WorkingDirectory = parameters.WorkingDirectory |
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 a bugfix as well, correct?
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.
Yes, changing the WorkingDirectory parameter was always ignored.
src/app/Fake.DotNet.Fsi/Fsi.fs
Outdated
@@ -75,8 +75,10 @@ type FsiParams = { | |||
(* - LANGUAGE - *) | |||
/// Generate overflow checks | |||
Checked: bool option | |||
/// Define a conditional compilation symbols | |||
/// Define a conditional compilation symbol |
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.
Yes indeed, I think we can't do that. But we can adjust documentation.
Sad that this slipped through in the process of moving to fake 5 ;)
Thanks! There should be a build on our staging environment shortly |
@marklam You can use/test 5.12.2-alpha.908 from the staging feed until there is a release on nuget |
Description
Fixes #2259 by adding
FsiParams.Definitions
to specify multiply symbols to be defined in Fsi.The
Define
parameter is left alone to avoid breaking the existing API.Also passes the
WorkingDirectory
value to the process.TODO
Documentation (in the form of the doc-comment for the parameter, and correcting the
unit or integration test exists
boy scout rule: "leave the code behind in a better state than you found it" (fix warnings, obsolete members or code-style in the places you worked in)
Fake 5 API guideline is honored