Implement "Add Open" code fix #2067

Merged
merged 13 commits into from Dec 21, 2016

Projects

None yet

9 participants

@vasily-kirichenko
Contributor

image

image

image

(It pops up assertion dialogs in Debug mode, so I recommend to test it in Release mode)

@vasily-kirichenko vasily-kirichenko port lexer symbol lookup logic from VFPT and use it in DocumentHighli…
…ghtsService
3d16bd4
@vasily-kirichenko vasily-kirichenko QuickInfoProvider uses lexer symbol c7bd6d5
@vasily-kirichenko vasily-kirichenko remove tryClassifyAtPosition 4664d14
@vasily-kirichenko vasily-kirichenko do not show quick info if FCS return a list of single FSharpTooltipEl…
…ement.None
4341c8c
@vasily-kirichenko vasily-kirichenko use standard Option combinators
remove Pervasive.fs
4876a01
@vasily-kirichenko vasily-kirichenko AddOpenCodeFixProvider WIP 08e2b8f
@vasily-kirichenko vasily-kirichenko Merge branch 'better-lexer-symbol' into add-open-code-fix 3da161b
@vasily-kirichenko vasily-kirichenko wip 6dba9b7
@vasily-kirichenko vasily-kirichenko do not use new functions from Option module introduced in F# 4.1 2743f42
@vasily-kirichenko vasily-kirichenko Merge branch 'master' into add-open-code-fix
Conflicts:
	vsintegration/src/FSharp.Editor/Common/CommonHelpers.fs
e0b6fe7
@vasily-kirichenko vasily-kirichenko it works 23a60ba
@vasily-kirichenko vasily-kirichenko cosmetics
3303f97
@forki
Contributor
forki commented Dec 21, 2016

💋

@forki
Contributor
forki commented Dec 21, 2016

(can you do the same for suggestions?)

@vasily-kirichenko
Contributor

@forki How they are presented? Just embedded into error messages as text?

@vasily-kirichenko
Contributor

@forki BTW, "Maybe you want ..." part looks unnatural in the code fix dialog :(

@forki
Contributor
forki commented Dec 21, 2016

@Krzysztof-Cieslak parses the error message in ionide and gives suggestions from it. There is probably a better way for VS.

@forki
Contributor
forki commented Dec 21, 2016

Regarding automatic open - @Krzysztof-Cieslak is implementing the very same feature for Ionide. can we find a way to share the code between these two. I mean afaik ionide needs it in fsautocomplete. How can we move forward?

@vasily-kirichenko
Contributor

We both ported the same my code from VFPT. I added the core logic into VFT-FCS, so it will be available in FCS package eventually.

@vasily-kirichenko vasily-kirichenko filter out duplicated open namespace code fixes
11143b5
@vasily-kirichenko
Contributor

@dotnet-bot test this please

@vasily-kirichenko
Contributor

1

@nosami
Member
nosami commented Dec 21, 2016

@vasily-kirichenko Awesome!

@vasily-kirichenko
Contributor

@nosami Yeah, this Code Fix UI is cool :)

@cartermp
Contributor

This is fantastic.

@KevinRansom
Contributor

The C# equivalent looks like:

foo

I prefer the C# wording, and I like that it offers the ability to add a using, add the full typename and also to generate a new type. It seems that if we have a fixer, we want one that offers a choice of fixes.

Kevin

@KevinRansom KevinRansom changed the title from Implement "Add Open" code fix to [WIP] Implement "Add Open" code fix Dec 21, 2016
@vasily-kirichenko
Contributor

I prefer the C# wording, and I like that it offers the ability to add a using, add the full typename and also to generate a new type. It seems that if we have a fixer, we want one that offers a choice of fixes.

About wording, it's the error message which our Diagnostics provider gives, we cannot control it.

This PR does provide two choices: open namespace/module or add full symbol name. About generating a type, I'm not sure it's necessary, I've never even thought about it.

@cloudRoutine
Contributor
cloudRoutine commented Dec 21, 2016 edited

Same for me, I cannot think of a time where generating a type was ever the solution I wanted for that kind of error state

@cartermp
Contributor

We could discuss what could or should be in the code fix UI in a separate discussion issue. For now, I think that the two namespace suggestion options are great experience-wise and fix a big problem today. The others are more along the lines of us looking to achieve experience parity with C#, and I'd rather have that as a part of a broader discussion.

@isaacabraham
Contributor

Let's start with the two we have. Can always iterate later to make it more powerful. The whole generate type in C# came as part of VS2010 IIRC for the TDD support it was pushing. I'm not sure how relevant it is for F#, plus you'd need to offer multiple types - Records, DUs and Classes I suppose.

@KevinRansom
Contributor

Sounds like a plan

@vasily-kirichenko thanks for this, it's nice

Kevin

@KevinRansom KevinRansom merged commit 5412d30 into Microsoft:master Dec 21, 2016

7 checks passed

Ubuntu14.04 Release Build Build finished.
Details
Windows_NT Debug Build Build finished.
Details
Windows_NT Release_ci_part1 Build Build finished.
Details
Windows_NT Release_ci_part2 Build Build finished.
Details
Windows_NT Release_ci_part3 Build Build finished.
Details
Windows_NT Release_net40_no_vs Build Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@KevinRansom KevinRansom changed the title from [WIP] Implement "Add Open" code fix to Implement "Add Open" code fix Dec 21, 2016
+[<RequireQualifiedAccess>]
+module Option =
+ /// Gets the value associated with the option or the supplied default value.
+ let inline getOrElse v = function
@KevinRansom
KevinRansom Dec 21, 2016 Contributor

Bugger these crept in again :-(

@dungpa
dungpa Dec 21, 2016 Contributor

@KevinRansom It's not your fault. We have to do it because the vsix isn't set up correctly to use the new core functions (see #2063).

@KevinRansom
KevinRansom Dec 21, 2016 Contributor

Well that's not great ....

@cloudRoutine
cloudRoutine Dec 21, 2016 Contributor

pasted image at 2016_12_20 20_48

I've gotten a bunch of these too

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