Add a codefix that suggests replacements for unknown identifiers #2106
Conversation
forki
commented
Dec 26, 2016
•
forki
commented
- - Suggestions for FS0039
- - Suggestions for other errors
- - Put strings into resx
|
@vasily-kirichenko could you please take a quick look if this goes into the right direction? @Krzysztof-Cieslak I think I need to change the error pattern a small bit. We should remember to adjust that next time when ionide updates FCS. |
forki
force-pushed
the
suggestfix
branch
2 times, most recently
from
9027078 to
4e52965
Compare
forki
changed the title
|
this is ready for review |
|
:)
|
|
LGTM |
| override __.RegisterCodeFixesAsync context : Task = | ||
| async { | ||
| context.Diagnostics | ||
| |> Seq.filter (fun x -> fixableDiagnosticIds |> List.contains x.Id) |
There was a problem hiding this comment.
Maybe make fixableDiagnosticIds a Set to faster filtering?
| context.Diagnostics | ||
| |> Seq.filter (fun x -> fixableDiagnosticIds |> List.contains x.Id) | ||
| |> Seq.iter (fun x -> | ||
| let splitted = x.GetMessage().Split([|"Maybe you want one of the following?"|], StringSplitOptions.None) |
There was a problem hiding this comment.
Is this string available as a constant somewhere in the compiler? Using it here as literal is fragile in case of future changes.
| let suggestions = | ||
| splitted.[1].Split([|' '; '\r'; '\n'|], StringSplitOptions.RemoveEmptyEntries) | ||
| |> Array.map (fun s -> s.Trim()) | ||
| let diagnostics = [| x |].ToImmutableArray() |
There was a problem hiding this comment.
Maybe choose a more descriptive name instead of x? The lambda is not one or two liner, so it's hard to track what x means.
There was a problem hiding this comment.
I think each code fix should contain all the diagnostic ids in its "fixing" list. Currently you generate a code fix for each id in context that this code fix supports.
There was a problem hiding this comment.
I think each code fix should contain all the diagnostic ids in its "fixing" list. Currently you generate a code fix for each id in context that this code fix supports.
what do I have to change?
There was a problem hiding this comment.
I was wrong, forget it.
| for suggestion in suggestions do | ||
| let codefix = | ||
| createCodeFix( | ||
| "Replace with " + suggestion, |
There was a problem hiding this comment.
"Replace with" should be put to the project's *.rc file, see how this is done in other code fixes.
| createCodeFix( | ||
| "Replace with " + suggestion, | ||
| context, | ||
| TextChange(TextSpan(context.Span.Start, context.Span.End), suggestion)) |
There was a problem hiding this comment.
TextSpan(context.Span.Start, context.Span.End) => context.Span
|
It still suggests unrelated stuff, like on the screenshot above. How on Earth |
|
It's close according to the given metric. There will always be false
positives...
Am 27.12.2016 15:29 schrieb "Vasily Kirichenko" <notifications@github.com>:
… It still suggests unrelated stuff, like on the screenshot above. How on
Earth nativeptr is close to DateT?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2106 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNMK216nf1Ud1SFIMxFPCcfZQpbeXks5rMSDCgaJpZM4LVuUq>
.
|
|
Have we moved over to Jaro Winkler? |
|
@isaacabraham yes long time ago |
| |> Seq.filter (fun x -> fixableDiagnosticIds |> Set.contains x.Id) | ||
| |> Seq.iter (fun diagnostic -> | ||
| let message = diagnostic.GetMessage() | ||
| let splitted = message.Split([|maybeString|], StringSplitOptions.None) |
There was a problem hiding this comment.
splitted is not really a word :-)
The past tense of split is .... split.
There was a problem hiding this comment.
https://en.m.wiktionary.org/wiki/splitted it says I'm nonstandard or archaic. Guess I can live with that.
Anyways, will fix tomorrow.
| |> Seq.iter (fun diagnostic -> | ||
| let message = diagnostic.GetMessage() | ||
| let splitted = message.Split([|maybeString|], StringSplitOptions.None) | ||
| if splitted.Length > 1 then |
There was a problem hiding this comment.
This feels somewhat icky ... we should consider adding to the compiler service an improved method of transmitting error information to the IDE.
For example:
- I'm not at all sure how this string parsing we do in these fixers would work on a language like Arabic which works left to right. (hmmm I meant right to left)
Obviously that's not specific to this PR ... but we should consider an improvement.
Kevin
There was a problem hiding this comment.
Absolutely. Parsing error messages is definitely the wrong way to do it (I think this specific case could also work in Arabic - because we concatenate the error message from left to right).
That said: it is an easy way to do it. And it works in practice. We already love to use it in ionide
…net#2106) * Add a codefix that suggests replacements for unknown identifiers * Use sets * Better naming * Change prediction text a bit to better work with MSBuild errors * cleanup * Suggest id if it is an infix
