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

[RFC] Add support for file operations in CodeActions #1766

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bstaletic
Copy link
Collaborator

@bstaletic bstaletic commented Oct 13, 2024

Tests will certainly fail, but I'd like to gather feedback before proceeding with this work.

Eventually fixes ycm-core/YouCompleteMe#4267

And of course, JDT was a special snowflake and had to break the protocol again.


This change is Reviewable

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)


ycmd/responses.py line 334 at r1 (raw file):

  def BuildFixitChunkData( chunk ):
    if hasattr( chunk, 'replacement_text' ):

minor, but maybe we should actually put the "to ycmd protocol" as a instance method on the chunk type.


ycmd/completers/language_server/language_server_completer.py line 3593 at r1 (raw file):
so previously we only ever processed one of these 'changes' vs 'documentChanges'.

Now we seem to include both if present.

This seems against the protocol which says:

A workspace edit represents changes to many resources managed in the workspace. The edit should either provide changes or documentChanges. If the client can handle versioned document edits and if documentChanges are present, the latter are preferred over changes.

So I think that we should prefer documentChanges.


ycmd/completers/language_server/language_server_completer.py line 3612 at r1 (raw file):

        chunks.append(
            responses.RenameChunk(
              old_filepath = lsp.UriToFilePath( text_document_edit[ 'oldUri' ] ),

If it recall correctly, this throws InvalidUriException in case that the server sends some bullshit URI like jdt:// and we have to catch it everywhere.


ycmd/completers/language_server/language_server_protocol.py line 310 at r1 (raw file):

        'dynamicRegistration': True
      },
      'workspaceEdit': {

we should consider specifying our FailureHandlingKind

   /**
    * The failure handling strategy of a client if applying the workspace edit
    * fails.
    *
    * @since 3.13.0
    */
   failureHandling?: FailureHandlingKind;

I guess export const Abort: FailureHandlingKind = 'abort'; is what we have.

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding "options", like these:

  1. https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#createFileOptions
  2. https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#renameFileOptions
  3. https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#deleteFileOptions

Should we just shove options with the chunks and pass them on to YCM?

Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)


ycmd/responses.py line 334 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

minor, but maybe we should actually put the "to ycmd protocol" as a instance method on the chunk type.

Done.


ycmd/completers/language_server/language_server_completer.py line 3593 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

so previously we only ever processed one of these 'changes' vs 'documentChanges'.

Now we seem to include both if present.

This seems against the protocol which says:

A workspace edit represents changes to many resources managed in the workspace. The edit should either provide changes or documentChanges. If the client can handle versioned document edits and if documentChanges are present, the latter are preferred over changes.

So I think that we should prefer documentChanges.

This is against the protocol, like I mentioned here:
ycm-core/YouCompleteMe#4267 (comment)

If you really think we should prefer documentChanges, I will do it.

JDT seems to send an empty changes and a populated documentChanges.


ycmd/completers/language_server/language_server_completer.py line 3612 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

If it recall correctly, this throws InvalidUriException in case that the server sends some bullshit URI like jdt:// and we have to catch it everywhere.

JDT has made that opt-in long ago.


ycmd/completers/language_server/language_server_protocol.py line 310 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

we should consider specifying our FailureHandlingKind

   /**
    * The failure handling strategy of a client if applying the workspace edit
    * fails.
    *
    * @since 3.13.0
    */
   failureHandling?: FailureHandlingKind;

I guess export const Abort: FailureHandlingKind = 'abort'; is what we have.

Done.

@bstaletic bstaletic force-pushed the documentChangesCodeAction branch from 86df022 to 39e7d9a Compare October 13, 2024 18:13
Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just shove options with the chunks and pass them on to YCM?

yeah why not. we might as well just use the LSP syntax and semantics for these; why swim against the tide.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, looking at the LSP spec, it's bonkers:

	/**
	 * Overwrite existing file. Overwrite wins over `ignoreIfExists`
	 */
	overwrite?: boolean;

	/**
	 * Ignore if exists.
	 */
	ignoreIfExists?: boolean;

erm... don't they mean the same thing!

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, they mean opposite things. If overwrite, then do not care that $FILE is there, just overwrite it. If ignoreIfExists, ignore the operation and leave that file alone.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, but I don't see how that's different from having just one of them. What am I missing!?

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

@bstaletic bstaletic force-pushed the documentChangesCodeAction branch from 39e7d9a to 16ea84f Compare October 13, 2024 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is there any plan to support workspace.workspaceEdit.resourceOperations
2 participants