Skip to content
This repository has been archived by the owner on Dec 19, 2024. It is now read-only.

flow suggest support? #151

Open
richardgirges opened this issue Aug 5, 2017 · 4 comments
Open

flow suggest support? #151

richardgirges opened this issue Aug 5, 2017 · 4 comments

Comments

@richardgirges
Copy link

First off, thanks for all the stellar work on this plugin! This package has singlehandedly convinced me to switch to VSCode full-time.

I apologize if a ticket already exists with this request, but I couldn't find one.

The flow suggest command is my go-to command for type annotation suggestions. Would love to see those suggestions pop up in the VSCode editor in-line. I also think it'll lend to the overall IDE-like feel of this plugin.

Any chance this enhancement could be made in the future?

Cheers

@richardgirges richardgirges changed the title flow suggest support? flow suggest" support? Aug 5, 2017
@richardgirges richardgirges changed the title flow suggest" support? flow suggest support? Aug 5, 2017
@orta
Copy link
Contributor

orta commented Aug 5, 2017

At this point the future of this extension is really inside the language server plugin ( see #150 ) - so making sure that the things you want to see are added at that level in flow, then exposed for consumers to use 👍

@wbinnssmith
Copy link

Yeah this would be so cool!

We'll need to implement this in flow-language-server (pull requests always welcome! ❤️).

I wonder which LSP API this could use. How do you imagine surfacing this in the UI?

Personally I think this would be awesome as a series of lint diagnostics of Hint (or maybe Info) severity, along with autofixes to implement annotating the suggestion with the strongest confidence.

It might also work as a CodeLens, but to me diagnostics makes more sense.

Once it gets implemented in flow-language-server, it should "just work" over here :)

What do you think?

cc @ljw1004

@ljw1004
Copy link

ljw1004 commented Aug 10, 2017

This is very similar to in Visual Studio/C# with its automatic type inference, similar to the auto specifier in C++.

var x = getString();  // some folks believe automatic inference is best-practice
string x = getString();  // some believe explicit types are better

There are differences in opinion about which is best. Visual Studio addresses this with refactorings a.k.a. code-fixes: you right-click on a specifier (be it var or string) and the popup menu invites you to (1) change this occurrence to the other style, (2) change all occurrences in the file to the other style. It also displays a diff preview of the change to let you evaluate.

Visual Studio has a nuanced take on how to indicate code-fixes visually to the user...

  • Some code-fixes are to fix errors or warnings; the error squiggle or its appearance in the error list lets the user know something's off.
  • Some code-fixes are to fix redundant code (e.g. an unnecessary cast, or unnecessary using statement); here Visual Studio colors the code slightly faded, so the user sees it's redundant. The user can delete it themselves or use a codefix.
  • Some code-fixes are just arbitrary matters of style (e.g. explicit vs inferred types, as is the subject of this thread). These would be too "in-your-face" to display as error diagnostics. Also there's no clear right answer so an error would be wrong. Instead, there is NO DIAGNOSTIC, but a code-fix is still available.

VSCode doesn't let you color the code text in a slightly faded-out way. But it does let you have code-fixes in any arbitrary location, including locations that aren't associated with errors/warnings. The way it does this is by sending the "get-available-codefixes" whenever the user moves their caret. (it's up to the language service to cache its computation so it can serve this quickly).

Personally, I'd love to see these as code-fixes, but I wouldn't like to see this as diagnostics. One of the files I'm working on has 71 flow suggestions, and none of them are ones that would be good to adopt, e.g.

-    this._stop().catch(_ => {}).then(_ => this._masterHost.dispose());
+    this._stop().catch(_ => : void{}).then(_: void => this._masterHost.dispose(): void);

-    const nextDisposable = new UniversalDisposable();
+    const nextDisposable: UniversalDisposable = new UniversalDisposable();

-    let response;
+    let response: Array<{newText: string, range: {end: {character: number, line: number}, start: {character: number, line: number}}}>;

I don't know if VSCode displays Info diagnostics by default in its error pane. I know that Atom does. All of these examples I think would be too much to show by default in the error pane.

A separate but related question is how to show the results of type-coverage flow coverage --color? The situation there is that even the best-intentioned person in the world can never get rid of all the type-coverage gaps (e.g. every catch block necessarily has any as the type of the exception, which always generates a type-coverage issue). If there's no way to improve your code to get rid of a warning, then that thing should not be showed as a warning.

@Mayank1791989
Copy link
Contributor

@richardgirges Partial support of this is added in flow v0.106 via codelens.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants