fix: Make project_id optional in results.py functions#204
fix: Make project_id optional in results.py functions#204sanjeed5 wants to merge 1 commit intogenerate-v1.xfrom
Conversation
project_id was a required positional argument in get_run_result(), get_run_metrics(), and compare_runs(), but none of them pass it to the backend - auth scopes handle project resolution. This caused TypeError when callers followed the docs/examples which omit it. Make project_id Optional[str] = None for backwards compatibility. Co-authored-by: Cursor <cursoragent@cursor.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
@claude review |
|
Claude finished @sanjeed5's task in 1m 21s —— View job Review of PR #204 complete
Result: This is a clean, low-risk fix. The change correctly makes One non-blocking suggestion posted as an inline comment: emit a |
| client: Any, # HoneyHive client | ||
| run_id: str, | ||
| project_id: str, | ||
| project_id: Optional[str] = None, |
There was a problem hiding this comment.
Suggestion (non-blocking): The docstring says "Deprecated" but no DeprecationWarning is emitted when a caller actually passes a value. For a published SDK, it's good practice to emit a warning so users know to update their code. Same applies to get_run_metrics and compare_runs.
| project_id: Optional[str] = None, | |
| project_id: Optional[str] = None, |
Consider adding something like this at the top of each function body:
if project_id is not None:
import warnings
warnings.warn(
"project_id is deprecated and ignored; backend uses auth scopes for project resolution.",
DeprecationWarning,
stacklevel=2,
)This isn't blocking — the current approach is already backwards-compatible and safe. But emitting a warning would help users discover they can simplify their call sites.
Review SummaryVerdict: Approve — This is a clean, well-motivated fix. The change is low-risk, fully backwards-compatible, and addresses a real What looks good
One suggestion (posted as inline comment)Since the docstrings say "Deprecated", consider emitting a DocumentationNo doc updates needed. The change makes the code match what the docs/examples already show (calling without |
Summary
get_run_result(),get_run_metrics(), andcompare_runs()inresults.pyall hadproject_idas a required positional argument, but none of them pass it to the backend (auth scopes handle project resolution).TypeErrorwhen calling these functions withoutproject_id, which is how the docs, Sphinx reference, and sample app all use them.project_id: strtoproject_id: Optional[str] = Nonein all three functions. Fully backwards compatible - existing callers that passproject_idstill work.Test plan
compare_runs(client, new_run_id, old_run_id)works withoutproject_idget_run_result(client, run_id)works withoutproject_idproject_id(e.g.core.py) still workMade with Cursor
Note
Low Risk
Signature/docstring-only change that preserves existing behavior and is unlikely to affect runtime beyond allowing more valid call patterns.
Overview
Makes
project_idan optional, deprecated parameter inget_run_result(),get_run_metrics(), andcompare_runs()so callers can omit it without hittingTypeError, while keeping backwards compatibility for existing call sites.Updates typing/imports and docstrings/examples to reflect that project resolution is handled by backend auth scopes and
project_idis accepted but ignored.Written by Cursor Bugbot for commit 9a019d6. This will update automatically on new commits. Configure here.