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

Add EnclosingEntity and IsAbstract to Declaration contract #129

Merged

Conversation

vasily-kirichenko
Copy link
Contributor

fsharp/fsharp-compiler-docs#650 must be merged and new FCS nuget package must be published and referenced in this PR before it's merged.

@vasily-kirichenko
Copy link
Contributor Author

It's ready for merging.

update integration tests output after updating FCS to version 8
@@ -91,6 +91,11 @@
<Choose>
<When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And $(TargetFrameworkVersion) == 'v4.5'">
<ItemGroup>
<Reference Include="FSharp.Compiler.Service.MSBuild.v12">
<HintPath>..\..\packages\FSharp.Compiler.Service\lib\net45\FSharp.Compiler.Service.MSBuild.v12.dll</HintPath>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have to add this DLL to your VSIX setup files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no VSIX in this project, it's just a library. However, I cannot find how the NuGet package is build and published (nothing in build.fsx)

Copy link
Member

Choose a reason for hiding this comment

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

There is no Nuget package, I think. Only GitHub release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ionide does not need nuget package, it just references the repository directly :)

@dsyme
Copy link
Contributor

dsyme commented Oct 14, 2016

@vasily-kirichenko Please keep an eye on how things stand w.r.t. MSBuild reference resolution etc. after this is integrated.

@vasily-kirichenko
Copy link
Contributor Author

I've no idea what's going on with the CIs. It builds OK on my machine.

@rneatherway
Copy link
Contributor

The Travis failure matches my experience running the integration tests against this PR locally on Linux. Why would FCS return different error messages on different platforms? Anyway, I am just looking at changing the test to use a user-defined symbol for which hopefully the results should be the same for both platforms.

@vasily-kirichenko
Copy link
Contributor Author

Yeah, FCS returns \r\n inside error messages on windows :(

@rneatherway
Copy link
Contributor

In this case the output seems semantically different:

-      "Message": "The value, constructor, namespace or type 'Directory' is not defined\r\nMaybe you want one of the following:\r\n   Direct\r\n   Ports\r\n   Compression\r\n   ErrorEventArgs\r\n   NotifyFilters",
+      "Message": "The value, constructor, namespace or type 'Directory' is not defined\nMaybe you want one of the following:\n   FileAction\n   FileData\n   Ports\n   kevent\n   timespec",

FCS suggests different replacements for the symbol.

@vasily-kirichenko
Copy link
Contributor Author

wow

@vasily-kirichenko
Copy link
Contributor Author

@rneatherway I tried to fix the line endings here 530f09e

@Krzysztof-Cieslak
Copy link
Member

It's not only about line endings, suggested types are different too

@vasily-kirichenko
Copy link
Contributor Author

@Krzysztof-Cieslak yes, I see. Very interesting. @forki @forki @forki

@rneatherway
Copy link
Contributor

A small change would remove the difference: #130

…claration-contract

Conflicts:
	test/FsAutoComplete.IntegrationTests/SymbolUse/output.json
@vasily-kirichenko
Copy link
Contributor Author

I've merged your change, let's see if it helps.

@rneatherway
Copy link
Contributor

Cool OK, I think it still bears investigation, but more at the FCS end than here.

@vasily-kirichenko
Copy link
Contributor Author

I think it's caused by new compiler error messages, so it's the compiler, not even FCS.

@vasily-kirichenko
Copy link
Contributor Author

I think I deserves merging now :)

@rneatherway rneatherway merged commit bd75a8e into ionide:master Oct 14, 2016
@rneatherway
Copy link
Contributor

Definitely!

@rneatherway
Copy link
Contributor

cheers

@vasily-kirichenko vasily-kirichenko deleted the enrich-declaration-contract branch October 28, 2016 13:09
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.

4 participants