-
Notifications
You must be signed in to change notification settings - Fork 878
Refactor CopilotClient initialization and enhance documentation #519
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
Open
brettcannon
wants to merge
12
commits into
github:main
Choose a base branch
from
brettcannon:api-changes
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
72041de
refactor(python): convert CopilotClient.__init__ from dict to keyword…
brettcannon 63b2238
feat(python): accept path-like objects in CopilotClient.__init__
brettcannon 816c396
docs(python): update README and exports for __init__ API changes
brettcannon 3835b34
Add @overload signatures to CopilotClient.__init__ for better type ch…
brettcannon 709ec4e
docs(python): document env param and nullable types in README
brettcannon 0563506
Merge remote-tracking branch 'upstream/main' into api-changes
brettcannon 11785ee
Merge branch 'main' into api-changes
brettcannon eda3e4f
refactor(tests): use os.fspath for Path-like arguments in TestPathLik…
brettcannon 537ed56
Fix a screw-up in a replay
brettcannon f90fe6a
docs: add cli_args option to CopilotClient documentation
brettcannon b161dd6
feat(tests): add os import for path handling in test_client.py
brettcannon d247f27
Fix formatting
brettcannon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -21,7 +21,7 @@ | |||||
| import threading | ||||||
| from dataclasses import asdict, is_dataclass | ||||||
| from pathlib import Path | ||||||
| from typing import Any, Callable, Optional, cast | ||||||
| from typing import Any, Callable, Optional, Union, cast, overload | ||||||
|
|
||||||
| from .generated.rpc import ServerRpc | ||||||
| from .generated.session_events import session_event_from_dict | ||||||
|
|
@@ -34,6 +34,7 @@ | |||||
| CustomAgentConfig, | ||||||
| GetAuthStatusResponse, | ||||||
| GetStatusResponse, | ||||||
| LogLevel, | ||||||
| ModelInfo, | ||||||
| PingResponse, | ||||||
| ProviderConfig, | ||||||
|
|
@@ -100,44 +101,106 @@ class CopilotClient: | |||||
| >>> await client.stop() | ||||||
|
|
||||||
| >>> # Or connect to an existing server | ||||||
| >>> client = CopilotClient({"cli_url": "localhost:3000"}) | ||||||
| >>> client = CopilotClient(cli_url="localhost:3000") | ||||||
| """ | ||||||
|
|
||||||
| def __init__(self, options: Optional[CopilotClientOptions] = None): | ||||||
| @overload | ||||||
| def __init__( | ||||||
| self, | ||||||
| *, | ||||||
| cli_url: str, | ||||||
| cwd: Union[str, os.PathLike[str], None] = None, | ||||||
| port: int = 0, | ||||||
| log_level: LogLevel = "info", | ||||||
| auto_start: bool = True, | ||||||
| auto_restart: bool = True, | ||||||
| env: Optional[dict[str, str]] = None, | ||||||
| ) -> None: ... | ||||||
|
|
||||||
| @overload | ||||||
| def __init__( | ||||||
| self, | ||||||
| *, | ||||||
| cli_path: Union[str, os.PathLike[str], None] = None, | ||||||
| cli_args: Optional[list[str]] = None, | ||||||
| cwd: Union[str, os.PathLike[str], None] = None, | ||||||
| port: int = 0, | ||||||
| use_stdio: Optional[bool] = None, | ||||||
| log_level: LogLevel = "info", | ||||||
| auto_start: bool = True, | ||||||
| auto_restart: bool = True, | ||||||
| github_token: Optional[str] = None, | ||||||
| use_logged_in_user: Optional[bool] = None, | ||||||
| env: Optional[dict[str, str]] = None, | ||||||
| ) -> None: ... | ||||||
|
|
||||||
| def __init__( | ||||||
| self, | ||||||
| *, | ||||||
| cli_path: Union[str, os.PathLike[str], None] = None, | ||||||
| cli_args: Optional[list[str]] = None, | ||||||
| cli_url: Optional[str] = None, | ||||||
| cwd: Union[str, os.PathLike[str], None] = None, | ||||||
| port: int = 0, | ||||||
| use_stdio: Optional[bool] = None, | ||||||
| log_level: LogLevel = "info", | ||||||
| auto_start: bool = True, | ||||||
| auto_restart: bool = True, | ||||||
| github_token: Optional[str] = None, | ||||||
| use_logged_in_user: Optional[bool] = None, | ||||||
| env: Optional[dict[str, str]] = None, | ||||||
| ): | ||||||
| """ | ||||||
| Initialize a new CopilotClient. | ||||||
|
|
||||||
| Args: | ||||||
| options: Optional configuration options for the client. If not provided, | ||||||
| default options are used (spawns CLI server using stdio). | ||||||
| cli_path: Path to the Copilot CLI executable. Accepts strings | ||||||
| or path-like objects. If not provided, uses the bundled | ||||||
| CLI binary. | ||||||
| cli_args: Additional command-line arguments to pass to the CLI | ||||||
| process. | ||||||
| cli_url: URL of an existing Copilot CLI server to connect to. | ||||||
| Format: "host:port", "http://host:port", or just "port". | ||||||
| Mutually exclusive with cli_path and use_stdio. | ||||||
| cwd: Working directory for the CLI process. Accepts strings | ||||||
| or path-like objects (default: current working directory). | ||||||
| port: Port for the CLI server in TCP mode (default: 0 for random). | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| use_stdio: Use stdio transport instead of TCP (default: True, | ||||||
| forced to False when cli_url is set). | ||||||
| log_level: Log level (default: "info"). | ||||||
| auto_start: Auto-start the CLI server on first use (default: True). | ||||||
| auto_restart: Auto-restart the CLI server if it crashes | ||||||
| (default: True). | ||||||
| github_token: GitHub token for authentication. Takes priority over | ||||||
| other authentication methods. | ||||||
| use_logged_in_user: Whether to use the logged-in user for | ||||||
| authentication (default: True, but False when github_token | ||||||
| is provided). Cannot be used with cli_url. | ||||||
| env: Environment variables for the CLI process. | ||||||
|
|
||||||
| Raises: | ||||||
| ValueError: If mutually exclusive options are provided (e.g., cli_url | ||||||
| with use_stdio or cli_path). | ||||||
| ValueError: If mutually exclusive options are provided (e.g., | ||||||
| cli_url with use_stdio or cli_path). | ||||||
|
|
||||||
| Example: | ||||||
| >>> # Default options - spawns CLI server using stdio | ||||||
| >>> client = CopilotClient() | ||||||
| >>> | ||||||
| >>> # Connect to an existing server | ||||||
| >>> client = CopilotClient({"cli_url": "localhost:3000"}) | ||||||
| >>> client = CopilotClient(cli_url="localhost:3000") | ||||||
| >>> | ||||||
| >>> # Custom CLI path with specific log level | ||||||
| >>> client = CopilotClient({ | ||||||
| ... "cli_path": "/usr/local/bin/copilot", | ||||||
| ... "log_level": "debug" | ||||||
| ... }) | ||||||
| >>> client = CopilotClient( | ||||||
| ... cli_path="/usr/local/bin/copilot", | ||||||
| ... log_level="debug", | ||||||
| ... ) | ||||||
| """ | ||||||
| opts = options or {} | ||||||
|
|
||||||
| # Validate mutually exclusive options | ||||||
| if opts.get("cli_url") and (opts.get("use_stdio") or opts.get("cli_path")): | ||||||
| if cli_url and (use_stdio or cli_path): | ||||||
| raise ValueError("cli_url is mutually exclusive with use_stdio and cli_path") | ||||||
|
|
||||||
| # Validate auth options with external server | ||||||
| if opts.get("cli_url") and ( | ||||||
| opts.get("github_token") or opts.get("use_logged_in_user") is not None | ||||||
| ): | ||||||
| if cli_url and (github_token or use_logged_in_user is not None): | ||||||
| raise ValueError( | ||||||
| "github_token and use_logged_in_user cannot be used with cli_url " | ||||||
| "(external server manages its own auth)" | ||||||
|
|
@@ -146,19 +209,19 @@ def __init__(self, options: Optional[CopilotClientOptions] = None): | |||||
| # Parse cli_url if provided | ||||||
| self._actual_host: str = "localhost" | ||||||
| self._is_external_server: bool = False | ||||||
| if opts.get("cli_url"): | ||||||
| self._actual_host, actual_port = self._parse_cli_url(opts["cli_url"]) | ||||||
| if cli_url: | ||||||
| self._actual_host, actual_port = self._parse_cli_url(cli_url) | ||||||
| self._actual_port: Optional[int] = actual_port | ||||||
| self._is_external_server = True | ||||||
| else: | ||||||
| self._actual_port = None | ||||||
|
|
||||||
| # Determine CLI path: explicit option > bundled binary | ||||||
| # Not needed when connecting to external server via cli_url | ||||||
| if opts.get("cli_url"): | ||||||
| if cli_url: | ||||||
| default_cli_path = "" # Not used for external server | ||||||
| elif opts.get("cli_path"): | ||||||
| default_cli_path = opts["cli_path"] | ||||||
| elif cli_path: | ||||||
| default_cli_path = os.fspath(cli_path) | ||||||
| else: | ||||||
| bundled_path = _get_bundled_cli_path() | ||||||
| if bundled_path: | ||||||
|
|
@@ -170,27 +233,25 @@ def __init__(self, options: Optional[CopilotClientOptions] = None): | |||||
| ) | ||||||
|
|
||||||
| # Default use_logged_in_user to False when github_token is provided | ||||||
| github_token = opts.get("github_token") | ||||||
| use_logged_in_user = opts.get("use_logged_in_user") | ||||||
| if use_logged_in_user is None: | ||||||
| use_logged_in_user = False if github_token else True | ||||||
|
|
||||||
| self.options: CopilotClientOptions = { | ||||||
| "cli_path": default_cli_path, | ||||||
| "cwd": opts.get("cwd", os.getcwd()), | ||||||
| "port": opts.get("port", 0), | ||||||
| "use_stdio": False if opts.get("cli_url") else opts.get("use_stdio", True), | ||||||
| "log_level": opts.get("log_level", "info"), | ||||||
| "auto_start": opts.get("auto_start", True), | ||||||
| "auto_restart": opts.get("auto_restart", True), | ||||||
| "cwd": os.fspath(cwd) if cwd else os.getcwd(), | ||||||
| "port": port, | ||||||
| "use_stdio": False if cli_url else (use_stdio if use_stdio is not None else True), | ||||||
| "log_level": log_level, | ||||||
| "auto_start": auto_start, | ||||||
| "auto_restart": auto_restart, | ||||||
| "use_logged_in_user": use_logged_in_user, | ||||||
| } | ||||||
| if opts.get("cli_args"): | ||||||
| self.options["cli_args"] = opts["cli_args"] | ||||||
| if opts.get("cli_url"): | ||||||
| self.options["cli_url"] = opts["cli_url"] | ||||||
| if opts.get("env"): | ||||||
| self.options["env"] = opts["env"] | ||||||
| if cli_args: | ||||||
| self.options["cli_args"] = list(cli_args) | ||||||
| if cli_url: | ||||||
| self.options["cli_url"] = cli_url | ||||||
| if env: | ||||||
| self.options["env"] = env | ||||||
| if github_token: | ||||||
| self.options["github_token"] = github_token | ||||||
|
|
||||||
|
|
@@ -273,7 +334,7 @@ async def start(self) -> None: | |||||
| RuntimeError: If the server fails to start or the connection fails. | ||||||
|
|
||||||
| Example: | ||||||
| >>> client = CopilotClient({"auto_start": False}) | ||||||
| >>> client = CopilotClient(auto_start=False) | ||||||
| >>> await client.start() | ||||||
| >>> # Now ready to create sessions | ||||||
| """ | ||||||
|
|
||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
Managing all these overloads feels painful but if this is what you recommend as the most idiomatic Python solution then I'll gladly defer to you.
Do IDEs really not provide good hinting when passing an object literal in as an argument?
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 don't quite follow what you mean by "good hinting when passing an object literal"? The overloads are there for type checkers because you have arguments that are valid only in certain situations (e.g.
cli_argsonly applies whencli_pathis provided).