feat: support resource operations#140
Conversation
|
Can you give some explanation of why this is needed? Examples of what this enables that doesn't currently work? |
|
@UziTech Sure! This enables resource operations (basically file and directory operations) as part of From https://microsoft.github.io/language-server-protocol/specification#workspaceEdit:
export interface WorkspaceEdit {
/**
* Holds changes to existing resources.
*/
changes?: { [uri: DocumentUri]: TextEdit[]; };
/**
* Depending on the client capability
* `workspace.workspaceEdit.resourceOperations` document changes are either
* an array of `TextDocumentEdit`s to express changes to n different text
* documents where each text document edit addresses a specific version of
* a text document. Or it can contain above `TextDocumentEdit`s mixed with
* create, rename and delete file / folder operations.
*
* Whether a client supports versioned document edits is expressed via
* `workspace.workspaceEdit.documentChanges` client capability.
*
* If a client neither supports `documentChanges` nor
* `workspace.workspaceEdit.resourceOperations` then only plain `TextEdit`s
* using the `changes` property are supported.
*/
documentChanges?: (
TextDocumentEdit[] |
(TextDocumentEdit | CreateFile | RenameFile | DeleteFile)[]
);
/**
* A map of change annotations that can be referenced in
* `AnnotatedTextEdit`s or create, rename and delete file / folder
* operations.
*
* Whether clients honor this property depends on the client capability
* `workspace.changeAnnotationSupport`.
*
* @since 3.16.0
*/
changeAnnotations?: {
[id: string /* ChangeAnnotationIdentifier */]: ChangeAnnotation;
};
} |
1dc357a to
949b5e4
Compare
949b5e4 to
643736c
Compare
|
@aminya looks like linting is failing because of an issue with pnpm. Could you look into that? |
Yes, I will fix that. |
c9a2cc7 to
ea1d628
Compare
|
@UziTech I implemented handling of resource operation options and added tests. |
ea1d628 to
b75cc53
Compare
| }) | ||
|
|
||
|
|
||
| it("throws an error on delete operation if target doesnt exist", async () => { |
There was a problem hiding this comment.
Should the delete operation be idempotent and fail silently if the file has already been deleted?
There was a problem hiding this comment.
I'm not sure.
LSP's CreateFile actually has an ignoreIfNotExists param.
What I implemented is:
- If
ignoreIfNotExistsisundefined, assumefalseand throw error - If
ignoreIfNotExistsistrue, return in case the file (or directory) doesn't exist
All ResourceOperations of such kind of params, notably overwrite, ignoreIfExists and ignoreIfNotExists.
I don't think that LSP gives any guidance on what should be considered the default value for such params, so I just assumed undefined === false, but I agree it might make sense to adapt with more sensible defaults...
WDYT?
There was a problem hiding this comment.
@aminya what do you think? I am on the fence on this one. On the one hand it makes sense to have undefined == false. On the other hand I don't see any situation where someone would want an error when deleting a file if the file already doesn't exist.
There was a problem hiding this comment.
I actually agree that a delete operation should not throw an error if the file doesn't exist.
For all other params/operation the sensible default would be false like :
- A
renameoperation should not overwrite another file and fail by default if the target exists (defaults:overwrite = false,ignoreIfExists = false - A
createoperation should not overwrite another file and fail by default if the target exists (defaults:overwrite = false,ignoreIfExists = false - A
deleteoperation should not fail by default if the target doesn't exist (defaults:ignoreIfNotExists = true<== our case here
So in case of a delete operation:
const ignoreIfNotExists = edit.options?.ignoreIfNotExists || trueAgreed?
There was a problem hiding this comment.
@UziTech This is what I did: https://github.com/atom-community/atom-languageclient/pull/140/files#diff-9011b4bf132f9c2bff2dfede53c468cea4b1710ce6937a8948a733ca1f109c52R99
Basically the default behavior is: don't throw an error when deleting a file that doesn't exist, except if the server requested ignoreIfNotExists: false.
dd5c5ec to
b29e99f
Compare
|
I changed the code to use |
f3d020e to
ed2d6a6
Compare
ed2d6a6 to
39f3e25
Compare
aminya
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks for your contribution!
|
🎉 This PR is included in version 1.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This is my first experience with typescript, feedback much appreciated ;)