Skip to content

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

Merged

Conversation

cdaguerre
Copy link
Contributor

This is my first experience with typescript, feedback much appreciated ;)

@UziTech
Copy link
Member

UziTech commented Apr 10, 2021

Can you give some explanation of why this is needed? Examples of what this enables that doesn't currently work?

@cdaguerre
Copy link
Contributor Author

@UziTech Sure!

This enables resource operations (basically file and directory operations) as part of workspaceEdits, which is currently not implemented at all.
I came across this while working on the rename functionality for the ide-intelephense package. If this PR makes it, i'll open another one to improve the rename functionality aswell (which to my understanding doesn't work at all right now).

From https://microsoft.github.io/language-server-protocol/specification#workspaceEdit:

Since version 3.13.0 a workspace edit can contain resource operations (create, delete or rename files and folders) as well. If resource operations are present clients need to execute the operations in the order in which they are provided. So a workspace edit for example can consist of the following two changes: (1) create file a.txt and (2) a text document edit which insert text into file a.txt.
And:

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;
	};
}

@cdaguerre cdaguerre force-pushed the feat/support-resource-operations branch from 1dc357a to 949b5e4 Compare April 12, 2021 18:04
@cdaguerre cdaguerre force-pushed the feat/support-resource-operations branch from 949b5e4 to 643736c Compare April 12, 2021 18:08
Copy link
Member

@UziTech UziTech left a 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

@UziTech
Copy link
Member

UziTech commented Apr 12, 2021

@aminya looks like linting is failing because of an issue with pnpm. Could you look into that?

@aminya
Copy link
Member

aminya commented Apr 12, 2021

@aminya looks like linting is failing because of an issue with pnpm. Could you look into that?

Yes, I will fix that.
https://github.com/atom-community/atom-ide-hyperclick/issues/132#issuecomment-818310509

@cdaguerre cdaguerre force-pushed the feat/support-resource-operations branch from c9a2cc7 to ea1d628 Compare April 13, 2021 12:42
@cdaguerre
Copy link
Contributor Author

@UziTech I implemented handling of resource operation options and added tests.
Rollbacks of applied resource operations in case of error are not handled for now but I think it should ok for a first iteration.

@cdaguerre cdaguerre force-pushed the feat/support-resource-operations branch from ea1d628 to b75cc53 Compare April 13, 2021 14:01
})


it("throws an error on delete operation if target doesnt exist", async () => {
Copy link
Member

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?

Copy link
Contributor Author

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 is undefined, assume false and throw error
  • If ignoreIfNotExists is true, 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?

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

@aminya aminya changed the title Support resource operations feat: support resource operations Apr 13, 2021
@aminya
Copy link
Member

aminya commented Apr 13, 2021

@aminya looks like linting is failing because of an issue with pnpm. Could you look into that?

#145 and #144 will fix the linting errors.

@cdaguerre cdaguerre force-pushed the feat/support-resource-operations branch from dd5c5ec to b29e99f Compare April 14, 2021 05:16
@cdaguerre
Copy link
Contributor Author

I changed the code to use fs.stats and fixed linting/format errors.
Build is green! 🎉

@aminya aminya added the enhancement New feature or request label Apr 14, 2021
@cdaguerre cdaguerre force-pushed the feat/support-resource-operations branch from f3d020e to ed2d6a6 Compare April 14, 2021 05:47
@cdaguerre cdaguerre force-pushed the feat/support-resource-operations branch from ed2d6a6 to 39f3e25 Compare April 14, 2021 05:50
Copy link
Member

@aminya aminya left a 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!

@aminya aminya requested a review from UziTech April 14, 2021 06:23
@UziTech UziTech merged commit 0c1c7e9 into atom-community:master Apr 14, 2021
@cdaguerre cdaguerre deleted the feat/support-resource-operations branch April 14, 2021 15:58
@github-actions
Copy link

🎉 This PR is included in version 1.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants