-
Notifications
You must be signed in to change notification settings - Fork 123
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] Add test for #748 #749
Conversation
The test is failing :( |
Wow, I can't believe this. Monkey patching worked first time! As @forki expected, FCS is not hitting the place where he put the fix for fcs.exe. After some navigation I copy & pasted the fix where it looked more plausible, but I'm not sure. @dsyme Could you please have a look? I don't know if I put the The best news is: it's working with Fable! Now I can go to sleep ;) |
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.
Don't forget to reintegrate that back to VF# again. Lol
A single (cached, may be discarded) instance of |
@vasily-kirichenko Thanks for the comment. I assume this also means that Now that you say that @forki, I realize that we may have the opposite problem: fsc doesn't hit the code in this PR. So we have two options:
|
I vote for keeping it as is in both places for now without making it DRY.
Tbh I think I also need to fix it in fsi. Will do that in separate pr
Am 01.04.2017 13:13 schrieb "Alfonso Garcia-Caro" <notifications@github.com
…:
@vasily-kirichenko <https://github.com/vasily-kirichenko> Thanks for the
comment. I assume this also means that IncrementalBuilder.ParseTask won't
be accessing seen from a different thread.
Now that you say that @forki <https://github.com/forki>, I realize that
we may have the opposite problem: fsc doesn't hit the code in this PR. So
we have two options:
- Keep the fix in the two places so it applies to both fsc and FCS. In
that case it may be a good idea to convert the deduplicate function
into a common helper (where?) parameterizing the seen dictionary.
- Try to move the code to CompileOps.ParseOneInputFile which seems to
be common place to obtain the ParsedInputs both in
dotnet/fsharp#2728
<dotnet/fsharp#2728> and this PR. In
that case we probably need to make seen a parameter of
ParseOneInputFile as this is a module helper and I guess it's not good
to make it dependent of a static mutable value.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#749 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNMd8GdTE_7YGgIlaehfwb6T0NXAlks5rrjFfgaJpZM4MwO5C>
.
|
So @dsyme please merge this one and @alfonsogcnunez please amend this to your VF# PR (so that VS tooling gets it as well). I will take care of fsi. |
I have published a new Fable version with a custom FCS binary and it's working for now. We can put this on hold until thinking a better way to consolidate the fix among FCS, fsc and fsi 👍 |
I cannot get the tests to run locally, neither for netcore or netfx, I see this:
Cannot run the new test in FSI either. I see this:
Let's see what CI says, then.