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

SemanticClassificationType should not be internal #696

Merged
merged 1 commit into from Feb 13, 2017

Conversation

Projects
None yet
6 participants
@cloudRoutine
Contributor

cloudRoutine commented Feb 11, 2017

In service.fsi SemanticClassificationType is marked internal yet

member GetSemanticClassification : unit -> (range * SemanticClassificationType)[]

is public and uses it as one of its return types, which makes this member unusable from FSAC and kills the colorization functionality.

@vasily-kirichenko

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Feb 11, 2017

Contributor

Yes, it should be fixed. FSAC has never had colorization functionality though.

Contributor

vasily-kirichenko commented Feb 11, 2017

Yes, it should be fixed. FSAC has never had colorization functionality though.

@cloudRoutine

This comment has been minimized.

Show comment
Hide comment
@cloudRoutine

cloudRoutine Feb 11, 2017

Contributor

we don't use it for Ionide, but it has been there. I don't know if the other editor plugins make use of it

member __.GetExtraColorizations = checkResults.GetExtraColorizationsAlternate()

there are a bunch of integration tests for them too

Contributor

cloudRoutine commented Feb 11, 2017

we don't use it for Ionide, but it has been there. I don't know if the other editor plugins make use of it

member __.GetExtraColorizations = checkResults.GetExtraColorizationsAlternate()

there are a bunch of integration tests for them too

@7sharp9

This comment has been minimized.

Show comment
Hide comment
@7sharp9

7sharp9 Feb 11, 2017

Member
Member

7sharp9 commented Feb 11, 2017

@vasily-kirichenko

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Feb 11, 2017

Contributor

The old method returned classification for computation expression names and CE custom operations usages only. It was used in VFT 2015, but not in VFPT ('coz why if VFT used it anyway and we could not switch it off). VFPT has richer colorization code, but I don't like to port it to VFT, VFT uses a bit lower level and 10x simpler approach (which is exposed as this new GetSemanticClassification method). I think it should be used by other editors (and FSharp.Formatting) instead of porting VFPT code.

Contributor

vasily-kirichenko commented Feb 11, 2017

The old method returned classification for computation expression names and CE custom operations usages only. It was used in VFT 2015, but not in VFPT ('coz why if VFT used it anyway and we could not switch it off). VFPT has richer colorization code, but I don't like to port it to VFT, VFT uses a bit lower level and 10x simpler approach (which is exposed as this new GetSemanticClassification method). I think it should be used by other editors (and FSharp.Formatting) instead of porting VFPT code.

@forki

Comment? Why not remove it completely?

@dsyme dsyme merged commit c28509d into fsharp:master Feb 13, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Krzysztof-Cieslak

This comment has been minimized.

Show comment
Hide comment
@Krzysztof-Cieslak

Krzysztof-Cieslak Feb 13, 2017

Contributor

@dsyme, can we get NuGet release of this fix as soon as possible? :)

Contributor

Krzysztof-Cieslak commented Feb 13, 2017

@dsyme, can we get NuGet release of this fix as soon as possible? :)

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Feb 13, 2017

Contributor

How about grabbing it off the AppVeyor build?

Contributor

dsyme commented Feb 13, 2017

How about grabbing it off the AppVeyor build?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Feb 13, 2017

Member
Member

forki commented Feb 13, 2017

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Feb 15, 2017

Member

Is this already merged back to Visualfsharp?

Member

forki commented Feb 15, 2017

Is this already merged back to Visualfsharp?

@vasily-kirichenko

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Feb 15, 2017

Contributor

@forki it's not needed there, but I think no.

Contributor

vasily-kirichenko commented Feb 15, 2017

@forki it's not needed there, but I think no.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Feb 15, 2017

Member

not needed - but we should keep it consistent. otherwise things will conflict and merge errors will happen. and doom.

so: Microsoft/visualfsharp#2440

Member

forki commented Feb 15, 2017

not needed - but we should keep it consistent. otherwise things will conflict and merge errors will happen. and doom.

so: Microsoft/visualfsharp#2440

dsyme added a commit to dsyme/FSharp.Compiler.Service that referenced this pull request Mar 29, 2017

fix fsi on Mono (fsharp#696)
* fix fsi

* fix fsi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment