Conversation
📝 WalkthroughWalkthroughThe CI and developer workflow were migrated to Makefile-driven targets: the single test job in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the reusable CI workflow to better align test job titles and to separate “non-video” vs “video” test execution based on which Stream credentials are required.
Changes:
- Renames/splits the prior
testjob intotest-non-videoandtest-videowith clearer job names per Python version. - Adjusts pytest invocation to exclude video-related code/tests from non-video runs and explicitly run video-related paths under video-enabled credentials.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
DEVELOPMENT.md (1)
58-61: Avoid documenting two primary codegen paths.This adds
make regen, but the existing## OpenAPI & Protobufsection later in the file still tells contributors to run./generate.shand./generate_webrtc.shdirectly. I'd collapse the lower section ontomake regenor explicitly label the scripts as the implementation behind that target, otherwise the guidance will drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DEVELOPMENT.md` around lines 58 - 61, Consolidate the duplicate codegen guidance by making "make regen" the single primary instruction and either remove or re-label the later "OpenAPI & Protobuf" section; update the text to state that "make regen" runs the underlying scripts (./generate.sh and ./generate_webrtc.sh) or collapse that section into the "Code generation" section so contributors only see one authoritative command (reference: the "make regen" mention and the ./generate.sh and ./generate_webrtc.sh script names).Makefile (1)
93-95: Consider pinningtyto improve reproducibility of this target.The
typechecktarget usesuvx ty check, which executestyin an isolated environment without version pinning. This differs from other quality targets in the Makefile that useuv runand benefit from exact version pinning inuv.lock. Iftyis already a dev dependency in the project, useuv run ty check ...to match the pattern and ensure reproducibility. Otherwise, pin the tool explicitly withuvx --from 'ty==…'to avoid fetching the latest version on first invocation.♻️ Suggested change if
tyis in dev dependencies## Run ty type checker typecheck: - uvx ty check getstream/ $(TY_EXCLUDES) + uv run ty check getstream/ $(TY_EXCLUDES)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 93 - 95, The Makefile's typecheck target uses an unpinned tool invocation ("uvx ty check" in the typecheck target), which makes runs non-reproducible; update the typecheck target to either call the pinned dev-dependency runner ("uv run ty check getstream/ $(TY_EXCLUDES)") if ty is already a dev dependency, or explicitly pin the tool when using uvx (e.g., use uvx --from 'ty==<version>' ty check getstream/ $(TY_EXCLUDES)) so the Makefile's typecheck target consistently uses a fixed ty version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@DEVELOPMENT.md`:
- Around line 3-22: Change the two top-level sections currently using "###"
headings ("### Setup" and "### Running tests") to "##" so they are siblings of
"## Release" and "## OpenAPI & Protobuf", and update the three fenced code
blocks under those headings to use shell language identifiers (```shell) so
markdown lint rules MD001/MD040 are satisfied; apply the same heading and fence
fixes to the other occurrences mentioned (the later blocks around the remainder
of the file).
---
Nitpick comments:
In `@DEVELOPMENT.md`:
- Around line 58-61: Consolidate the duplicate codegen guidance by making "make
regen" the single primary instruction and either remove or re-label the later
"OpenAPI & Protobuf" section; update the text to state that "make regen" runs
the underlying scripts (./generate.sh and ./generate_webrtc.sh) or collapse that
section into the "Code generation" section so contributors only see one
authoritative command (reference: the "make regen" mention and the ./generate.sh
and ./generate_webrtc.sh script names).
In `@Makefile`:
- Around line 93-95: The Makefile's typecheck target uses an unpinned tool
invocation ("uvx ty check" in the typecheck target), which makes runs
non-reproducible; update the typecheck target to either call the pinned
dev-dependency runner ("uv run ty check getstream/ $(TY_EXCLUDES)") if ty is
already a dev dependency, or explicitly pin the tool when using uvx (e.g., use
uvx --from 'ty==<version>' ty check getstream/ $(TY_EXCLUDES)) so the Makefile's
typecheck target consistently uses a fixed ty version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89015bc9-7ef0-4ce1-a965-056f3f9b890f
📒 Files selected for processing (4)
.github/workflows/run_tests.ymlDEVELOPMENT.mdMakefiledev.py
✅ Files skipped from review due to trivial changes (1)
- dev.py
| ### Setup | ||
|
|
||
| Clone the repo and sync | ||
| Clone the repo and install dependencies: | ||
|
|
||
| ``` | ||
| git clone git@github.com:GetStream/stream-py.git | ||
| uv sync --no-sources --all-packages --all-extras --dev | ||
| cp .env.example .env # fill in your Stream credentials | ||
| pre-commit install # optional: enable commit hooks | ||
| ``` | ||
|
|
||
| Env setup | ||
| ### Running tests | ||
|
|
||
| Run `make help` to see all available targets. The most common ones: | ||
|
|
||
| ``` | ||
| cp .env.example .env | ||
| make test # non-video tests (chat, feeds, moderation, etc.) | ||
| make test-video # video/WebRTC tests only | ||
| make test-all # both of the above | ||
| ``` |
There was a problem hiding this comment.
Fix the new Markdown hierarchy/language lint regressions.
These sections read as top-level siblings of ## Release and ## OpenAPI & Protobuf, so they should be ## headings instead of ###. The added command snippets are also all shell examples, so fencing them as shell will clear the MD001/MD040 warnings and keep the document structure consistent.
Also applies to: 27-64
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 18-18: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@DEVELOPMENT.md` around lines 3 - 22, Change the two top-level sections
currently using "###" headings ("### Setup" and "### Running tests") to "##" so
they are siblings of "## Release" and "## OpenAPI & Protobuf", and update the
three fenced code blocks under those headings to use shell language identifiers
(```shell) so markdown lint rules MD001/MD040 are satisfied; apply the same
heading and fence fixes to the other occurrences mentioned (the later blocks
around the remainder of the file).
Summary by CodeRabbit