refactor(core/cli): change 'zed-integration' to be ACP focused #11074
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @kastkeepitjumpinlikekangaroos, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses issue #11026 by performing a significant refactoring of the codebase. The primary goal is to generalize the existing 'Zed integration' to an 'ACP integration,' acknowledging that the functionality now supports a wider range of platforms beyond Zed. This involves renaming directories, files, functions, and configuration parameters to use 'ACP' instead of 'Zed,' ensuring the codebase accurately reflects its current capabilities. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the 'Zed integration' to 'ACP integration' by renaming files, functions, and variables across the cli and core packages. The changes are consistent and align with the goal of generalizing the feature.
However, the refactoring introduces a breaking API change in the @google/gemini-cli-core package by renaming experimentalZedIntegration to experimentalACPIntegration in the exported ConfigParameters interface. I've added a high-severity comment with a detailed suggestion on how to handle this, either by documenting it as a breaking change or by implementing a backward-compatible deprecation cycle to ensure a smooth transition for consumers of the core package.
| extensionContextFilePaths?: string[]; | ||
| maxSessionTurns?: number; | ||
| experimentalZedIntegration?: boolean; | ||
| experimentalACPIntegration?: boolean; |
There was a problem hiding this comment.
This change from experimentalZedIntegration to experimentalACPIntegration in the exported ConfigParameters interface constitutes a breaking API change for the @google/gemini-cli-core package. Any external consumer of this package that constructs a Config object will have their code broken.
If this is an intentional breaking change, it should be clearly documented in the pull request description and reflected in the package's versioning (e.g., a major version bump).
If you wish to avoid a breaking change, you could maintain backward compatibility for a deprecation cycle. Here's an example of how you could do that:
-
Update
ConfigParametersto accept both properties:// packages/core/src/config/config.ts export interface ConfigParameters { // ... experimentalACPIntegration?: boolean; /** @deprecated Use experimentalACPIntegration instead. Will be removed in a future version. */ experimentalZedIntegration?: boolean; // ... }
-
Update the
Configconstructor to handle both:// packages/core/src/config/config.ts // In Config constructor this.experimentalACPIntegration = params.experimentalACPIntegration ?? params.experimentalZedIntegration ?? false;
-
Keep the old getter with a
@deprecatedtag:// packages/core/src/config/config.ts // In Config class getExperimentalACPIntegration(): boolean { return this.experimentalACPIntegration; } /** @deprecated Use getExperimentalACPIntegration instead. Will be removed in a future version. */ getExperimentalZedIntegration(): boolean { return this.experimentalACPIntegration; }
This approach would provide a smoother transition for the package consumers.
There was a problem hiding this comment.
Looking for maintainer advice on which option is prefered for the project. Happy to make the associated change!
…ntegration now that there is wider support for ACP outside of Zed.
e5ceafb to
f33a750
Compare
TLDR
Fix #11026 by renaming the Zed integraion to ACP integration now that there is wider support for ACP outside of Zed.
npm run preflightis passing locally:system report:
Dive Deeper
Reviewer Test Plan
Testing Matrix
Linked issues / bugs
Fixes #11026