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

Merge Ionide changes upstream #110

Merged
merged 13 commits into from
Jun 20, 2016
Merged

Merge Ionide changes upstream #110

merged 13 commits into from
Jun 20, 2016

Conversation

Krzysztof-Cieslak
Copy link
Member

  1. Naive support for project.json (this probably will be dropped in futture but let's have it now)
  2. Better (file) paths normalization across different features
  3. Resolve scripts to latest .Net on Windows
  4. Make completion faster on Suave
  5. Depend on F# 4 (FSharp.Core deployed with application) instead of 4.3.1

Lot of changes (sorry :( ) but all "production" tested on Ionide users ;)

@rneatherway
Copy link
Contributor

All changes look good to me! Just a bit concerned about the failing test:

--- a/test/FsAutoComplete.IntegrationTests/MultipleUnsavedFiles/output.json
+++ b/test/FsAutoComplete.IntegrationTests/MultipleUnsavedFiles/output.json
@@ -48,16 +48,5 @@
 }
 {
   "Kind": "errors",
-  "Data": [
-    {
-      "FileName": "<absolute path removed>/FsAutoComplete.IntegrationTests/MultipleUnsavedFiles/Program.fs",
-      "StartLine": 1,
-      "EndLine": 1,
-      "StartColumn": 23,
-      "EndColumn": 29,
-      "Severity": "Error",
-      "Message": "The value, constructor, namespace or type 'addTwo' is not defined",
-      "Subcategory": "typecheck"
-    }
-  ]
+  "Data": []
 }

It's for the unsaved files, I wonder why that could be broken.

@Krzysztof-Cieslak
Copy link
Member Author

That's pretty weird. I can't reproduce this failing test on my machine (Windows)

@rneatherway
Copy link
Contributor

Funny, it does here on OSX, I'll try to work out why.

@rneatherway
Copy link
Contributor

So it turns out it is due to the new drive letter normalization you introduced. I did some instrumentation and found the following:

+[getFile] Looking for: C:\projects\fsautocomplete\test\FsAutoComplete.IntegrationTests\MultiProj\Proj2\Core.fs
+[getFile] Currently known: seq
+  ["c:\projects\fsautocomplete\test\FsAutoComplete.IntegrationTests\MultiProj\Proj1\Ops.fs";
+   "c:\projects\fsautocomplete\test\FsAutoComplete.IntegrationTests\MultiProj\Proj1\Program.fs";
+   "c:\projects\fsautocomplete\test\FsAutoComplete.IntegrationTests\MultiProj\Proj2\Core.fs";
+   "c:\projects\fsautocomplete\test\FsAutoComplete.IntegrationTests\MultiProj\Proj2\Program.fs"]

What is that normalization needed for? If it is definitely required then it needs to be applied to FileSystem.fs requests as well else this feature is broken.

@Krzysztof-Cieslak
Copy link
Member Author

From what I remember, Atom returns paths with small letter for drive, VS Code with capital letter (Or other way round). I've thought it would be good idea to get it normalized in FSAC so it can work for any editor without any additional work on editor side.

@rneatherway
Copy link
Contributor

I see, OK, well if you propagate that through to FileSystem.fs that should fix this regression.

@Krzysztof-Cieslak
Copy link
Member Author

Hey, thanks for help, added normalization there.

@rneatherway rneatherway merged commit dd974e7 into ionide:master Jun 20, 2016
@Krzysztof-Cieslak Krzysztof-Cieslak deleted the dotnetcore branch June 20, 2016 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants