-
Notifications
You must be signed in to change notification settings - Fork 13
feat: support resource operations #140
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
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM just needs tests
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the delete operation be idempotent and fail silently if the file has already been deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure.
LSP's CreateFile
actually has an ignoreIfNotExists
param.
What I implemented is:
- If
ignoreIfNotExists
isundefined
, assumefalse
and throw error - If
ignoreIfNotExists
istrue
, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
rename
operation should not overwrite another file and fail by default if the target exists (defaults:overwrite = false
,ignoreIfExists = false
- A
create
operation should not overwrite another file and fail by default if the target exists (defaults:overwrite = false
,ignoreIfExists = false
- A
delete
operation 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 || true
Agreed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ;)