-
Notifications
You must be signed in to change notification settings - Fork 772
Fix Python SDK type issues with ty type checker #1122
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
base: main
Are you sure you want to change the base?
Conversation
Resolve 43 type diagnostics reported by ty (Astral's Python type checker). Changes include: - Fixed Self type on class singletons to use concrete forward references - Added explicit type annotations for instance attributes shadowing static methods - Replaced None with UNSET sentinel for optional auto-generated API parameters - Fixed protocol mismatches and method signatures to align types - Added type: ignore suppressions for class_method_variant overload patterns and protocol structural subtyping edge cases All checks pass: ty check, ruff format, ruff check. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: be1f50b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The @overload declarations mixing @staticmethod/@classmethod with regular methods are fundamentally invalid in Python's type system. Remove them and keep only the @class_method_variant implementation. The runtime behavior (calling as both instance and class method) is unchanged via the descriptor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix DockerfileParserInterface protocol: simplify copy.src to str (matching actual usage in parse_dockerfile), remove unused CopyItem import - Fix metadata type: use cast(Dict[str, str], ...) for auto-generated Union[Unset, Any] field after isinstance narrowing - Fix mcp type: use cast(Any, mcp) to bridge SDK McpServer type with auto-generated McpType0 (different representations of same data) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…d-override The child classes' _cls_connect returns Self while the parent returns the API Sandbox model. Renaming avoids the incompatible override. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract Unset isinstance checks into local variables for readability. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The downstream SandboxOpts types it as Optional[str] and checks `is not None`, so an empty string would incorrectly pass through. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| }, | ||
| headers=headers, | ||
| token=token, | ||
| token=token or "", |
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.
this is fine
E2B/packages/python-sdk/e2b/api/client/client.py
Lines 265 to 267 in 12c8d6e
| self._headers[self.auth_header_name] = ( | |
| f"{self.prefix} {self.token}" if self.prefix else self.token | |
| ) |
Use self: str overloads to tell the type checker that methods like
Sandbox.kill("sandbox_id") and sandbox.kill() are both valid call
patterns. This avoids mixing @classmethod with instance method
overloads (which ty rejects as invalid-overload).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Switch from `self: str` overloads to `@classmethod` overloads so basedpyright can resolve the class method variants. Configure ty to ignore `invalid-overload` since the pattern is intentional. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Switch from @classmethod to @staticmethod on overloads to match the original code. @staticmethod doesn't add cls parameter so the parameter count stays consistent with the implementation, avoiding basedpyright's reportInconsistentOverload. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The class_method_variant pattern (dual instance/static dispatch) is inherently inexpressible in Python's type system, so the overload implementation can never match the static overload signature. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add a CI workflow that runs type checking on PRs, mirroring the lint workflow. Add typecheck scripts to all packages (tsc for JS/TS, ty for Python). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…in permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
- Suppress no-matching-overload globally (ty bug with @staticmethod overloads) - Add per-path overrides for generated API client models and protobuf stubs - Add type annotations to test data dicts to fix union type inference - Add inline ty:ignore for pytest.skip false positives - Remove now-redundant inline suppression comments Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Revert `mcp` from `is not None` check back to truthiness check so empty dicts are treated as UNSET. Remove unnecessary local alias in get_sandbox_url and use self._sandbox_url directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
5b8435b to
be1f50b
Compare
Summary
All checks pass: ty check, ruff format, ruff check.
Note
Medium Risk
Mostly type-safety and CI additions, but it touches sandbox connection/creation code paths and subtly changes accepted/returned types (e.g., optional tokens/domains and UNSET vs None), which could affect API behavior if assumptions were relied on.
Overview
Adds a new PR typechecking gate and wires up monorepo
typecheckscripts. A newTypecheckGitHub Action runspnpm run typecheck, with package-leveltypecheckscripts added for the JS SDK, CLI, and Python SDK.Introduces
tyfor the Python SDK and fixes ty-reported typing issues. This addstyas a dev dependency plusty/basedpyright config and amake typechecktarget, and updates Python SDK code to satisfy stricter typing (e.g., removingSelfsingletons, making some tokens/domainsOptional, aligning overloads via@staticmethod+ renamed_cls_connect_sandbox, casting generated-model fields/params, usingUNSETinstead ofNonefor optional API query params, and adding targetedtyignores in tests).Written by Cursor Bugbot for commit be1f50b. This will update automatically on new commits. Configure here.