Skip to content
This repository was archived by the owner on Mar 5, 2026. It is now read-only.

Migrate textDocument/*#2633

Merged
mkondratek merged 4 commits intomainfrom
mkondratek/chore/migrate-api-part-2
Nov 13, 2024
Merged

Migrate textDocument/*#2633
mkondratek merged 4 commits intomainfrom
mkondratek/chore/migrate-api-part-2

Conversation

@mkondratek
Copy link
Copy Markdown
Contributor

@mkondratek mkondratek commented Nov 13, 2024

This PR is a part of the protocol migration. Some endpoints are written by hand. We are switching to the protocol generated from Cody. In this PR:

Based on PR #2632

Full chain of PRs as of 2024-11-13

Test plan

  • Verified with debugger - the proper values are sent
  • Tested with current selection in chat, tested with autocompletion, tested with inline edits - works

@mkondratek mkondratek self-assigned this Nov 13, 2024
@mkondratek mkondratek changed the title mkondratek/chore/migrate-api-part-2 Migrate textDocument/* Nov 13, 2024
val contentChanges: List<ProtocolTextDocumentContentChangeEvent>? = null,
val testing: TestingParams? = null,
) {
class ProtocolTextDocumentUtil {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Again, we may want to reconsider the naming. But I'd rather migrate the API quickly and apply the other improvements later.

ideVersion = ApplicationInfo.getInstance().build.toString(),
workspaceRootUri =
ProtocolTextDocument.normalizeUriOrPath(
ProtocolTextDocumentUtil.normalizeUriOrPath(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not put it in the protocol_extensions and use the existing extension pattern?
Then you won't need all those renames.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These methods are not extension functions in Kotlin sense. However, I did some refactor and it should look better now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In particular. I cannot define the other class named ProtocolTextDocument since there is one generated already.

@mkondratek mkondratek requested a review from pkukielka November 13, 2024 10:04
Base automatically changed from mkondratek/chore/migrate-api-part-1 to main November 13, 2024 10:10
@mkondratek mkondratek force-pushed the mkondratek/chore/migrate-api-part-2 branch from 63afb4c to 673dacb Compare November 13, 2024 10:19
@mkondratek mkondratek force-pushed the mkondratek/chore/migrate-api-part-2 branch from 673dacb to f20f828 Compare November 13, 2024 10:20
@@ -0,0 +1,6 @@
package com.sourcegraph.cody.agent.protocol_extensions

object TestingParamsUtil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could also be TestingParamsExt for consistency

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown
Contributor

@pkukielka pkukielka left a comment

Choose a reason for hiding this comment

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

LGTM

@mkondratek mkondratek enabled auto-merge (squash) November 13, 2024 10:28
@mkondratek mkondratek merged commit 29514fe into main Nov 13, 2024
@mkondratek mkondratek deleted the mkondratek/chore/migrate-api-part-2 branch November 13, 2024 10:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants