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

Proposal for codefix commands #34787

Closed
ghost opened this issue Sep 21, 2017 · 10 comments · Fixed by #35002
Closed

Proposal for codefix commands #34787

ghost opened this issue Sep 21, 2017 · 10 comments · Fixed by #35002
Assignees
Labels
api on-testplan plan-item VS Code - planned item for upcoming under-discussion Issue is under discussion for relevance, priority, approach
Milestone

Comments

@ghost
Copy link

ghost commented Sep 21, 2017

This would enable microsoft/TypeScript#14423.
I am mostly describing changes to protocol.ts in the TypeScript repository here.
@mjbvz @egamma @mhegazy we discussed this today.

1

The returned CodeAction in a code action request may include a list of commands.

export interface CodeAction {
    description: string;
    changes: FileCodeEdits[];
    command?: {};
}

A code action may even have an empty changes list and consist of only a command.
We could choose specify that command must an of commands, although I'm not sure if that's worth bothering with.

2

If the code action is accepted, the editor must send the commands back to the language service to actually be carried out. This means needing a new request type, "applyCodeFixCommand". This looks like:

export interface ApplyCodeFixCommandRequest extends Request {
    command: CommandTypes.ApplyCodeFixCommand;
    arguments: ApplyCodeFixCommandRequestArgs;
}

export interface ApplyCodeFixCommandRequestArgs extends FileRequestArgs {
    command: {};
}

The contents of command will just be what was sent to the editor.
(Typically this will include a "type" field so the language service can dispatch on the type of action.)

3

The language service should send a response when the action is complete.

export interface ApplyCodeFixCommandResponse extends Response {
    body: {};
}

I'm thinking we can just reuse the message field for error reporting and for a successful response. (Currently the field is documented to only be used if success is false.) We could put that in the body, but then it seems redundant.

@vscodebot vscodebot bot added the api label Sep 21, 2017
@mjbvz mjbvz self-assigned this Sep 21, 2017
@mjbvz
Copy link
Collaborator

mjbvz commented Sep 21, 2017

This proposed TS API changes seem good to me. I don't see any issues implementing this on our side. We'll just need to figure out how best to surface the success / failure messages to users

Let me know when the TS part lands in a nightly build so that I can try prototyping this

@mjbvz mjbvz added the under-discussion Issue is under discussion for relevance, priority, approach label Sep 21, 2017
@ghost
Copy link
Author

ghost commented Sep 22, 2017

If we put this in a nightly build, people will start seeing codefixes that don't work. Could you check out a branch instead?

@mjbvz
Copy link
Collaborator

mjbvz commented Sep 22, 2017

Sure, sounds good

@ghost
Copy link
Author

ghost commented Sep 25, 2017

@mjbvz You can checkout and build the install_types_fix branch from the TypeScript repository.
If you include nonsense; in a file and it is an unused identifier, we'll give you a code fix that comes with a command to write a file nonsense.d.ts. (It also replaces the file content so you can verify that at least that works.)

@mjbvz
Copy link
Collaborator

mjbvz commented Sep 25, 2017

Thanks @andy-ms. I'll open a PR on our side shortly. A few quick notes:

Looks like ApplyCodeFixCommandRequestArgs is currently using action instead of command for the key. The command name makes more sense to me

Also, I get this error when I try to run the command:

[Trace  - 4:01:56 PM] Sending request: applyCodeFixCommand (15). Response expected: yes. Current queue length: 0
Arguments: {
    "file": "/Users/matb/projects/sand/y.ts",
    "action": {
        "type": "nonsense"
    }
}

Error: EISDIR: illegal operation on a directory, open '/Users/matb/projects/san'
    at Object.fs.openSync (fs.js:584:18)
    at Object.module.(anonymous function) [as openSync] (ELECTRON_ASAR.js:173:20)
    at Object.writeFile (/Users/matb/projects/TypeScript/built/local/tsserver.js:3963:30)
    at Object.ts.sys.sys.writeFile (/Users/matb/projects/TypeScript/built/local/tsserver.js:4226:37)
    at LSHost.writeFile (/Users/matb/projects/TypeScript/built/local/tsserver.js:95670:27)
    at Object.applyCodeFixCommand (/Users/matb/projects/TypeScript/built/local/tsserver.js:93685:30)
    at IOSession.Session.applyCodeFixCommand (/Users/matb/projects/TypeScript/built/local/tsserver.js:100557:67)
    at Session.handlers.ts.createMapFromTemplate._a.(anonymous function) (/Users/matb/projects/TypeScript/built/local/tsserver.js:99436:40)
    at /Users/matb/projects/TypeScript/built/local/tsserver.js:100701:88
    at IOSession.Session.executeWithRequestId (/Users/matb/projects/TypeScript/built/local/tsserver.js:100692:28)
    at IOSession.Session.executeCommand (/Users/matb/projects/TypeScript/built/local/tsserver.js:100701:33)
    at IOSession.Session.onMessage (/Users/matb/projects/TypeScript/built/local/tsserver.js:100721:35)
    at Interface.<anonymous> (/Users/matb/projects/TypeScript/built/local/tsserver.js:101943:27)
    at emitOne (events.js:96:13)
    at Interface.emit (events.js:191:7)
    at Interface._onLine (readline.js:241:10)
    at Interface._normalWrite (readline.js:384:12)
    at Socket.ondata (readline.js:101:10)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:191:7)
    at readableAddChunk (_stream_readable.js:178:18)
    at Socket.Readable.push (_stream_readable.js:136:10)
    at Pipe.onread (net.js:560:20)

@mjbvz mjbvz added this to the October 2017 milestone Sep 25, 2017
@mjbvz
Copy link
Collaborator

mjbvz commented Sep 25, 2017

Here's the PR: #35002

@ghost
Copy link
Author

ghost commented Sep 26, 2017

@mjbvz I pushed a quick fix of those two issues -- changed the name from "action" to "command" and remembered to write to a file rather than to a directory.

@mjbvz
Copy link
Collaborator

mjbvz commented Sep 26, 2017

Ok, the fix looks good. I see the d.ts file being created now

@ghost
Copy link
Author

ghost commented Sep 26, 2017

Any reason this couldn't land in nightly so I can experiment with it? Regular users won't see anything until we actually add something in TypeScript master that uses this.

@mjbvz
Copy link
Collaborator

mjbvz commented Sep 26, 2017

We're getting ready for release currently but I can check this in once we branch off for the September release. This code in its current state won't compile until we pick up a new *.d.ts file from TypeScript, but we can sprinkle in some any casts to work around this until then

@mjbvz mjbvz added the plan-item VS Code - planned item for upcoming label Oct 11, 2017
mjbvz added a commit to mjbvz/vscode that referenced this issue Oct 20, 2017
Initial prototype to support commands on JS/TS quick fixes

Part of microsoft#34787
mjbvz added a commit that referenced this issue Oct 20, 2017
* Prototype TS Codefix commands

Initial prototype to support commands on JS/TS quick fixes

Part of #34787

* use command for argument name

* Update to use published api
@vscodebot vscodebot bot locked and limited conversation to collaborators Dec 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api on-testplan plan-item VS Code - planned item for upcoming under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant