Changes for supporting resource level operations #295

gorkem · 2017-12-22T01:15:44Z

LSP proposal that adds resourceChanges to WorkspaceEditor to allow manipulation of files and folders. This pull request only includes the proposed changes. It is not possible to implement these changes for vscode due to lack of APIs. This PR addresses microsoft/language-server-protocol#272

dbaeumer · 2017-12-22T08:03:05Z

Thanks a lot for the proposal. When creating a new file shouldn't we provide an option for the content. I would ensure that we can create a file with a predefined content. Since stuff is async we couldn't otherwise.

And do we need to change something for the response to provide better error information. For example a file couldn't be created?

rcjsuen · 2017-12-22T08:25:14Z

protocol/src/protocol.resourceChanges.proposed.md

+_resourceChanges_
+
+A workspace edit can optionally provide resource changes. If present resource changes are applied before the `changes/documentChanges` has been applied
+allowing the `change/documentChanges` to work on affected files. The changes are applied in the order that they are supplied.


This changes/documentChanges confused me as it made it seem like a notification or a request (like a textDocument/didOpen). Perhaps it might be better to spell it out specifically with "changes or documentChanges" instead?

rcjsuen · 2017-12-22T08:25:30Z

protocol/src/protocol.resourceChanges.proposed.md

+export interface ResourceChange{
+
+  /**
+  * The resource change operation type


Period at the end?

rcjsuen · 2017-12-22T08:26:29Z

protocol/src/protocol.resourceChanges.proposed.md

+  * The resource.
+  *
+  * If change type is CREATE a new file is created on the uri.
+  * This operation will not overwrite any existing resources and will fail.


I think this needs to be cleared up to state that the operation will fail if the path conflicts with an existing resource.

rcjsuen · 2017-12-22T08:27:23Z

protocol/src/protocol.resourceChanges.proposed.md

+  * This operation will not overwrite any existing resources and will fail.
+  *
+  * If change type is DELETE the resource is deleted if it exists. If the
+  * uri is a folder this operation deletes all its children.


Is it considered a success or a failure if you try to delete a file or folder that does not exist?

rcjsuen · 2017-12-22T08:27:41Z

protocol/src/protocol.resourceChanges.proposed.md

+  * The new uri for the resource pointed by path.
+  * Required only if change type is MOVE. Must be compatible with the path
+  * uri ie. must be a file uri if path is a file uri.
+  *


Unnecessary extra space.

rcjsuen · 2017-12-22T08:27:55Z

protocol/src/protocol.resourceChanges.proposed.ts

+export interface ResourceChange{
+
+  /**
+  * The resource change operation type


rcjsuen · 2017-12-22T08:28:00Z

protocol/src/protocol.resourceChanges.proposed.ts

+  * The resource.
+  *
+  * If change type is CREATE a new file is created on the uri.
+  * This operation will not overwrite any existing resources on this uri.


rcjsuen · 2017-12-22T08:28:05Z

protocol/src/protocol.resourceChanges.proposed.ts

+  * This operation will not overwrite any existing resources on this uri.
+  *
+  * If change type is DELETE the resource is deleted if it exists. If the
+  * uri is a folder this operation deletes all its children.


rcjsuen · 2017-12-22T08:28:09Z

protocol/src/protocol.resourceChanges.proposed.ts

+  * The new uri for the resource pointed by path.
+  * Required only if change type is MOVE. Must be compatible with the path
+  * uri ie. must be a file uri if path is a file uri.
+  *


rcjsuen · 2017-12-22T08:29:28Z

protocol/src/protocol.resourceChanges.proposed.md

+workspace: {
+  workspaceEdit?: {
+		  /**
+		  * The client supports resource changes in `WorkspaceEdit`s


Need a period at the end.

rcjsuen · 2017-12-22T08:29:41Z

protocol/src/protocol.resourceChanges.proposed.md

+
+_Client Capabilities_:
+
+The client sets the following capability if it is supporting resource Changes.


Is there a particular reason as to why "Changes" is capitalized?

rcjsuen · 2017-12-22T08:32:39Z

protocol/src/protocol.resourceChanges.proposed.ts

+/**
+ * A resource change type
+ */
+export declare namespace ResourceChangeType{


Same as above.

rcjsuen · 2017-12-22T08:32:48Z

protocol/src/protocol.resourceChanges.proposed.ts

+ * ------------------------------------------------------------------------------------------ */
+'use strict'
+
+export interface ResourceChange{


Same as above.

rcjsuen · 2017-12-22T08:33:00Z

protocol/src/protocol.resourceChanges.proposed.md

+/**
+ * A resource change type
+ */
+export declare namespace ResourceChangeType{


Should add a space after ResourceChangeType to be consistent with the other files.

rcjsuen · 2017-12-22T08:33:08Z

protocol/src/protocol.resourceChanges.proposed.md

+Changes to a folder or file on the workspace.
+
+```ts
+export interface ResourceChange{


Should add a space after ResourceChange to be consistent with the other files.

rcjsuen · 2017-12-22T08:33:46Z

protocol/src/protocol.resourceChanges.proposed.md

+```ts
+interface  WorkspaceEdit{
+
+  resourceChanges?: ResourceChange[]


Since we talk about changes and documentChanges in a WorkspaceEdit, would it be better to also list those properties again here? I'm not sure what would be best since we're extending the interface here (by adding optional properties) without actually creating a new interface. Thoughts, @dbaeumer?

rcjsuen · 2017-12-22T08:34:01Z

protocol/src/protocol.resourceChanges.proposed.md

+allowing the `change/documentChanges` to work on affected files. The changes are applied in the order that they are supplied.
+
+```ts
+interface  WorkspaceEdit{


Should add a space after WorkspaceEdit to be consistent with the other files.

gorkem · 2017-12-22T13:51:28Z

@dbaeumer I thought the initial content could be provided via changes or documentChanges filed on WorkspaceEdit. resourceChanges are defined to run before TextEdits are applied. Do you think this would not be enough?

gorkem · 2017-12-22T22:52:30Z

Thanks @rcjsuen very helpful review. I think I have addressed most of them.

gorkem · 2017-12-22T22:57:45Z

@dbaeumer On error information with the response: I think we need to address that on workspace/applyEdit and textDocument/rename which are the users of WorkspaceEdit.

gorkem · 2018-01-28T22:49:24Z

Updated the proposed ResourceChanges so that it no longer requires additional enumeration. vscode's corresponding implementation gives a chance to define an (almost) ordered execution of the text and resource changes but I could not see a way to define it on WorkspaceEdit without breaking or completely defining a new field.

dbaeumer · 2018-02-01T14:53:25Z

@gorkem @rcjsuen thanks again for providing this. Please note that VS Code 1.20 will not ship with a resource change API due to problems we discovered with the API during the test pass (see the corresponding vscode issue).

We discussed in detail the order issue and it is our belief that we need one array describing all changes to make the behavior correct.

For the LSP I would do the following: if resource changes are supported the workspace edit result has a property resourceChanges which are either a rename, move, delete or content change.

gorkem · 2018-02-12T23:45:55Z

@dbaeumer did a small change to allow ordered resourceChanges, please take a look if it looks OK, I want to proceed with the implementation.

spoenemann · 2018-04-03T07:19:00Z

The type

interface  WorkspaceEdit {
    resourceChanges?: (ResourceChange | TextDocumentEdit)[];
}

is hard to realize in Java because both ResourceChange and TextDocumentEdit are objects, so the only way to create the correct class instance is to look ahead in the JSON token stream, which means we cannot use the default stream parsing approach of the Gson library. See eclipse-lsp4j/lsp4j#173.

I'd propose to split this:

interface  WorkspaceEdit {
    resourceChanges?: ResourceChange[];
    textEdits?: TextDocumentEdit[];
}

dbaeumer · 2018-04-03T11:00:36Z

@gorkem regarding: #295 (comment). I think that is not a good idea (e.g. assuming that resource changes are applied before text edits). This would mean it is harder to express to modify the content of a file and then move. The statement should be that at the edits should be one array (but we need to address @spoenemann concern) and that they are executed in order in which they are provided.

gorkem · 2018-04-12T22:01:06Z

@dbaeumer I am not sure if there is a great benefit of having a single ordered list of resource changes and text edits. A WorkspaceEdit is essentially a single unit and it should be fully applied or failed. I think the proposed two arrays is equivalent to having a single array.
What is the use case that you have in mind that requires a single ordered list?

dbaeumer · 2018-04-26T13:17:29Z

The case is you create a file and then afterwards want to edit its content.

gorkem · 2018-04-27T02:47:51Z

The original proposal was to execute the resource changes before TextEdits which covers the create file case. The only case I see is modify contents and delete case which would become delete and modify in which case the implementations should drop the TextEdits

dbaeumer · 2018-04-27T07:24:32Z

Agree that this would work as well. But why making it more complicated to explain to end users. Having one array which is executed from 0..n is pretty simple to explain. I agree it needs more work on the implementation side.

spoenemann · 2018-04-27T07:33:45Z

An alternative solution for the Java problem would be to let ResourceChange extend Partial< TextDocumentEdit> so the WorkspaceEdit can be written as

interface  WorkspaceEdit {
    resourceChanges?: ResourceChange[];
}

spoenemann · 2018-04-27T07:35:11Z

...or simply redeclare the properties of TextDocumentEdit as optional in ResourceChange.

Add resourceChanges to WorkspaceEditor to allow manipulation of files and folders.

gorkem · 2018-05-01T20:24:20Z

Let's give this another try; uploaded a new change. Now ResourceChange includesTextDocumentEdit functionality. It now supports ordering and allows initial content for create operations.

dbaeumer · 2018-05-08T08:43:25Z

I see the problem with mapping JSON to Java however the current proposal requires IMO a lot of documentation since it is not obvious what it does. Problems I see:

the textDocument property for a resource level operation (e.g. create, move, delete) doesn't need to be a VersionedDocumentIdentifier. It might make sense for move or delete but i might be hard to servers to construct this we we create a file, then change it and then move it.
it gives the impression that only text documents could be move, created or deleted

Would a type attribute on the resource chnage help the Java library to implement the right switch early instead if having to check properties ?

spoenemann · 2018-05-09T11:57:22Z

Would a type attribute on the resource chnage help the Java library to implement the right switch early instead if having to check properties ?

But then we would still need to parse the type attribute before we can decide which class instance to create?

To be clear: parsing based on attributes / properties is possible also in Java / LSP4J, it just increases the effort for implementing such a parser and is slower at runtime. If the best solution in terms of LSP data structure design requires this, we'll manage to implement it, but it would be good to avoid it if there are acceptable alternatives for the data structure design.

dbaeumer · 2018-05-09T12:09:03Z

@spoenemann thanks for the clarification.

gorkem · 2018-06-26T16:34:24Z

Since this has stalled a bit because I got new responsibilities, @tsmaeder will be taking over this PR, so stay tuned

spoenemann · 2018-08-17T09:42:11Z

@dbaeumer @gorkem @tsmaeder I found a solution for parsing object type alternatives in eclipse-lsp4j/lsp4j#230 that is acceptable in terms of additional required code for each such case. From the perspective of LSP4J, we can leave the original declaration

interface  WorkspaceEdit {
    resourceChanges?: (ResourceChange | TextDocumentEdit)[];
}

as it is, since we can now parse it correctly.

dbaeumer · 2018-08-21T14:49:00Z

@spoenemann thanks

dbaeumer · 2018-08-28T10:24:50Z

Closing PR. Resource edit support is available in latest next releases.

gorkem changed the title ~~Changes for supporting resource level changes~~ Changes for supporting resource level operations Dec 22, 2017

rcjsuen reviewed Dec 22, 2017

View reviewed changes

gorkem force-pushed the resourceChanges branch 2 times, most recently from d8bc061 to 9f6df8f Compare December 22, 2017 22:51

gorkem force-pushed the resourceChanges branch from 9f6df8f to a85d858 Compare January 28, 2018 22:42

gorkem force-pushed the resourceChanges branch from a85d858 to e790be9 Compare February 12, 2018 23:42

gorkem mentioned this pull request Feb 23, 2018

Add file level operations to WorkspaceEdit eclipse-lsp4j/lsp4j#163

Merged

Ruin0x11 mentioned this pull request Mar 4, 2018

Support for renaming/moving files Ruin0x11/intellij-lsp-server#4

Open

Changes for supporting resource level changes

744dcd0

Add resourceChanges to WorkspaceEditor to allow manipulation of files and folders.

gorkem force-pushed the resourceChanges branch from e790be9 to 744dcd0 Compare May 1, 2018 20:21

egamma mentioned this pull request Jul 9, 2018

Iteration Plan for July 2018 microsoft/vscode#53850

Closed

49 tasks

Ruin0x11 mentioned this pull request Jul 23, 2018

Rename command Ruin0x11/intellij-lsp-server#72

Open

dbaeumer closed this Aug 28, 2018


		_Client Capabilities_:

		The client sets the following capability if it is supporting resource Changes.

Changes for supporting resource level operations #295

Changes for supporting resource level operations #295

Conversation

gorkem commented Dec 22, 2017

dbaeumer commented Dec 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gorkem commented Dec 22, 2017 • edited Loading

gorkem commented Dec 22, 2017

gorkem commented Dec 22, 2017

gorkem commented Jan 28, 2018

dbaeumer commented Feb 1, 2018

gorkem commented Feb 12, 2018

spoenemann commented Apr 3, 2018

dbaeumer commented Apr 3, 2018

gorkem commented Apr 12, 2018

dbaeumer commented Apr 26, 2018

gorkem commented Apr 27, 2018

dbaeumer commented Apr 27, 2018

spoenemann commented Apr 27, 2018

spoenemann commented Apr 27, 2018

gorkem commented May 1, 2018

dbaeumer commented May 8, 2018 • edited Loading

spoenemann commented May 9, 2018

dbaeumer commented May 9, 2018

gorkem commented Jun 26, 2018

spoenemann commented Aug 17, 2018

dbaeumer commented Aug 21, 2018

dbaeumer commented Aug 28, 2018

gorkem commented Dec 22, 2017 •

edited

Loading

dbaeumer commented May 8, 2018 •

edited

Loading