-
Notifications
You must be signed in to change notification settings - Fork 585
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
WIP - Fsi module #1970
WIP - Fsi module #1970
Conversation
I think we can have a uniform API to use across Also I'm completely happy if we skip
If you decide to keep the Regarding the external process I think we need to look at some examples how this new API is used. Ideally - according to API guidelines - we should open Fake.DotNet
Fsi.exec (fun p -> { w with ... }) But it seems currently we need to (or "can do" depending on the view) open Fake.DotNet.Fsi
Exe.execFsiSimple ... Which is a bit unusual to how other modules work. I guess we should do some minor refactorings there in order to get the above. What do you think? At the very least we should probably add some example usages in the in-line comments to guide users into the "proper"/"suggested" usage scenario. |
I agree, the current usage is definitely a little awkward. I wish f# allowed modules to be split between files haha, that would have alleviated some of weirdness of its current usage. For now, I'll work to get it back to a more consistent usage of just |
Hey @matthid, I think I've fixed up the API a lot better now. I also added in the FCS runner, but I don't totally know if this is working correctly or not since I believe it doesn't work on netcore yet. I did get some scripts to run in fsi successfully but I was getting weird errors regarding cli arguments and things of that sort. Also I'm not sure if it's missing something in comparison to the old yaaf implementation. I've left it commented out for now but if this all seems good to you, I can uncomment it and finish the process of adding this module to the set of fake 5 modules |
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, can you add src/app/Fake.DotNet.Fsi/Fake.DotNet.Fsi.fsproj
to Fake.sln
(otherwise I'm not sure it is built at all)
Hey @matthid: I think this is ready. The checks are failing for the 'Tests' target, I don't believe its related to my changes. Also, I apologize for the mixed up commits, they got a little messed up it seems. My rebase was acting weird and I couldn't figure out how to get it working without overwriting my commits. Let me know if I need to put any more effort towards fixing it or if its ok as is. |
changed the base to release/next as there was a problem with some tests, can you commit a dummy commit to trigger CI (or rebase)? |
src/app/Fake.DotNet.Fsi/Fsi.fs
Outdated
let argls s (ls:string list) = stringEmptyMap (sprintf "--%s:\"%s\"" s) (String.concat ";" ls) | ||
/// format a compiler arg that ends with "+" or "-" with string parameters "--%s%s:\"%s\"" | ||
let inline toglls s b (ls:'a list) = | ||
stringEmptyMap (sprintf "--%s%s:\"%s\"" s (chk b)) (String.concat ";" (List.map string ls)) |
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 guess this doesn't work, have you tried that?
(because args.ToWindowsCommandLine
is doing the escaping FSI will get a literal quote character).
I think we can just remove the quotes from the sprintf.
I guess the same is for argls
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 I guess I didn't test that one. Ill give it a look
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.
It looks like it actually works either way, literal quotes in the argument or not -- but I removed them anyways
BTW: I don't know if arguments are passed into the internal FCS version of fsi the same way they are with the cli. I'm basically passing them in the same except I don't include the script to be run in the arguments since thats passed to FsiSession.EvalScript
function. Now that I think of it, I didn't run FsiSession in its own process either...
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 have expected the quotes to be "quoted" again later, strange
Thanks! After looking at this several times I'll probably change some details:
What do you think? |
That all sounds good to me. I had thought about the param to switch between external and internal previously, I just was debating with myself if keeping fsiparams specifically for fsi args was better for design |
@vanceism7 Please consider reviewing 575330d |
I'll try and check it out hopefully later today or sometime this weekend. Thanks for your help! @matthid |
@matthid: This module is basically finished (depending on your view point), I just have a few questions to clear up. Fsi module is essentially built the same exact way the fsc module is. There were two sets of code here, one to run an external exe and one to run the internal fsi from fsharp compiler service. I think it may be better to split these into two modules such as
DotNet.Fsi.Exe
andDotNet.Fsi.Service
, but if consistency is more important, I can put both inside a basicDotNet.Fsi
module like we did withDotNet.Fsc
. If we do take the two module route, do you have any naming suggestions? Calls to Exe.function don't look super great in a script since Exe is very vague. Not sure how I should name these if we keep it as two.Also, as was mentioned previously, the internal FCS version of fsi is not currently working in .net core so should we just release the external exe functionality for now or should both be added before releasing this module?