hubcio commented on code in PR #3825:
URL: https://github.com/apache/iggy/pull/3825#discussion_r3727691601
##########
foreign/python/README.md:
##########
@@ -38,33 +39,111 @@ pip install apache-iggy
### Local Development
-```bash
-# Start server for testing using docker
-docker compose -f docker-compose.test.yml up --build
-
-# Or use cargo
-cargo run --bin iggy-server -- --with-default-root-credentials --fresh
-
-# Using uv:
-uv sync --all-extras
-uv run maturin develop
-uv run pytest tests/ -v # Run tests (requires iggy-server running)
-
-# Using pip:
-python3 -m venv .venv
-source .venv/bin/activate
-pip install -e ".[all]"
-maturin develop
-pytest tests/ -v # Run tests (requires iggy-server running)
-```
+1. Build a project for development
+
+ With `uv`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > uv venv
+ > ```
+ >
+ > Sync the environment without updating it:
+ >
+ > ```bash
+ > uv sync --frozen --all-extras --no-install-project
+ > ```
+ >
+ > Build the project - this runs cargo build and performs an editable
install:
+ >
+ > ```bash
+ > uv run maturin develop
+ > ```
+
+ With `pip`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > python3 -m venv .venv
+ > ```
+ >
+ > Activate the venv:
+ >
+ > ```bash
+ > source .venv/bin/activate
+ > ```
+ >
+ > Install the dependencies with `pip`:
+ >
+ > ```bash
+ > pip install -e ".[all]"
Review Comment:
pip path lost its rebuild step - master had `maturin develop` after `pip
install -e ".[all]"`, this rewrite dropped it. there's no pure-python source
here; the module is the compiled cdylib, and the editable install copies the
built `.so` into site-packages (`direct_url.json` says editable, but there's no
`.pth` redirect and no import hook - the artifact is a snapshot). so after any
rust edit, `pytest` silently tests the old binary. a new symbol at least fails
loud with `AttributeError`, but changed behavior on an existing method just
runs stale code, and the fix-a-bug-then-retest loop is actively misleading -
the test keeps failing and you start "fixing" code that was already correct.
re-add an explicit rebuild command to the pip path.
##########
foreign/python/README.md:
##########
@@ -38,33 +39,111 @@ pip install apache-iggy
### Local Development
-```bash
-# Start server for testing using docker
-docker compose -f docker-compose.test.yml up --build
-
-# Or use cargo
-cargo run --bin iggy-server -- --with-default-root-credentials --fresh
-
-# Using uv:
-uv sync --all-extras
-uv run maturin develop
-uv run pytest tests/ -v # Run tests (requires iggy-server running)
-
-# Using pip:
-python3 -m venv .venv
-source .venv/bin/activate
-pip install -e ".[all]"
-maturin develop
-pytest tests/ -v # Run tests (requires iggy-server running)
-```
+1. Build a project for development
+
+ With `uv`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > uv venv
+ > ```
+ >
+ > Sync the environment without updating it:
+ >
+ > ```bash
+ > uv sync --frozen --all-extras --no-install-project
+ > ```
+ >
+ > Build the project - this runs cargo build and performs an editable
install:
+ >
+ > ```bash
+ > uv run maturin develop
+ > ```
+
+ With `pip`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > python3 -m venv .venv
+ > ```
+ >
+ > Activate the venv:
+ >
+ > ```bash
+ > source .venv/bin/activate
+ > ```
+ >
+ > Install the dependencies with `pip`:
+ >
+ > ```bash
+ > pip install -e ".[all]"
+ > ```
+
+2. Run the server to be able to run the tests
+
+ ```bash
+ cargo run --bin iggy-server -- --with-default-root-credentials --fresh
Review Comment:
step 2 only works from repo root - `foreign/python` is excluded from the
root workspace, so from the sdk dir cargo errors with `no bin target named
'iggy-server'`. meanwhile steps 1/3/4 only work from `foreign/python`. no step
says where to run it from, so the numbered sequence can't be followed from any
single directory - a per-command cwd note would fix the whole thing. two more
small things here: the server runs foreground (steps 3-5 need a second
terminal), and `--fresh` wipes `local_data/` on every start (the flag's own
help says "THIS WILL DELETE ALL DATA!") - worth a word of warning now that it's
the only documented way to start the server.
##########
foreign/python/README.md:
##########
@@ -24,6 +24,7 @@ Apache Iggy is the persistent message streaming platform
written in Rust, suppor
```bash
# Using uv
+uv venv # if not already created
Review Comment:
`uv venv` isn't the missing prerequisite here - in a bare directory `uv add
apache-iggy` fails with `No pyproject.toml found in current directory or any
parent directory`, venv or not (`uv add` needs a project). since this readme is
also the pypi landing page, better to show `uv pip install apache-iggy` for the
standalone flow and drop this line. `uv add` is fine when the reader already
has a project.
##########
foreign/python/README.md:
##########
@@ -38,33 +39,111 @@ pip install apache-iggy
### Local Development
-```bash
-# Start server for testing using docker
-docker compose -f docker-compose.test.yml up --build
-
-# Or use cargo
-cargo run --bin iggy-server -- --with-default-root-credentials --fresh
-
-# Using uv:
-uv sync --all-extras
-uv run maturin develop
-uv run pytest tests/ -v # Run tests (requires iggy-server running)
-
-# Using pip:
-python3 -m venv .venv
-source .venv/bin/activate
-pip install -e ".[all]"
-maturin develop
-pytest tests/ -v # Run tests (requires iggy-server running)
-```
+1. Build a project for development
+
+ With `uv`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > uv venv
+ > ```
+ >
+ > Sync the environment without updating it:
+ >
+ > ```bash
+ > uv sync --frozen --all-extras --no-install-project
+ > ```
+ >
+ > Build the project - this runs cargo build and performs an editable
install:
+ >
+ > ```bash
+ > uv run maturin develop
+ > ```
+
+ With `pip`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > python3 -m venv .venv
+ > ```
+ >
+ > Activate the venv:
+ >
+ > ```bash
+ > source .venv/bin/activate
+ > ```
+ >
+ > Install the dependencies with `pip`:
+ >
+ > ```bash
+ > pip install -e ".[all]"
+ > ```
+
+2. Run the server to be able to run the tests
+
+ ```bash
+ cargo run --bin iggy-server -- --with-default-root-credentials --fresh
+ ```
+
+3. Run the tests
+
+ `uv`:
+
+ ```bash
+ uv run --no-sync pytest tests/ -v
+ ```
+
+ `pip`:
+
+ ```bash
+ pytest tests/ -v # make sure iggy-server is running and the venv is
activated
+ ```
+
+4. To update the stubs, use
+
+ ```bash
+ cargo run --bin stub_gen
+ ```
+
+5. Before committing, test the pre-commit and pre-push hooks. `prek` only
inspects staged content, so stage your work first:
+
+ ```bash
+ git add -A
+ prek run # runs pre-commit hooks
+ prek run --hook-stage pre-push
+ ```
+
+ These are some of the essential commands prek is running, so it's
recommended to run them manually before running prek / committing / pushing.
This list is not exhaustive and other hook failures are possible.
+
+ ```bash
+ ruff format .
+ ```
+
+ ```bash
+ ruff check --fix .
+ ```
+
+ ```bash
+ cargo fmt --all
+ ```
+
+ ```bash
+ cargo clippy --all-targets --all-features -- -D warnings
Review Comment:
also line 129. neither command covers this crate from repo root -
`foreign/python` is excluded from the workspace, so `cargo fmt --all` is a
silent no-op for the sdk and this clippy line builds the whole root workspace
while checking none of the sdk's rust. the prek hooks have the same blind spot
(they run against the root manifest), so while the text above is accurate about
what prek runs, the pair still never lints this crate. CI does it right: `cd
foreign/python` then `cargo fmt --manifest-path Cargo.toml` / `cargo clippy
--manifest-path Cargo.toml --all-targets --all-features -- -D warnings`. worth
using those forms here and noting they run from `foreign/python`.
##########
foreign/python/README.md:
##########
@@ -38,33 +39,111 @@ pip install apache-iggy
### Local Development
-```bash
-# Start server for testing using docker
-docker compose -f docker-compose.test.yml up --build
-
-# Or use cargo
-cargo run --bin iggy-server -- --with-default-root-credentials --fresh
-
-# Using uv:
-uv sync --all-extras
-uv run maturin develop
-uv run pytest tests/ -v # Run tests (requires iggy-server running)
-
-# Using pip:
-python3 -m venv .venv
-source .venv/bin/activate
-pip install -e ".[all]"
-maturin develop
-pytest tests/ -v # Run tests (requires iggy-server running)
-```
+1. Build a project for development
+
+ With `uv`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > uv venv
+ > ```
+ >
+ > Sync the environment without updating it:
+ >
+ > ```bash
+ > uv sync --frozen --all-extras --no-install-project
+ > ```
+ >
+ > Build the project - this runs cargo build and performs an editable
install:
+ >
+ > ```bash
+ > uv run maturin develop
+ > ```
+
+ With `pip`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > python3 -m venv .venv
+ > ```
+ >
+ > Activate the venv:
+ >
+ > ```bash
+ > source .venv/bin/activate
+ > ```
+ >
+ > Install the dependencies with `pip`:
+ >
+ > ```bash
+ > pip install -e ".[all]"
+ > ```
+
+2. Run the server to be able to run the tests
+
+ ```bash
+ cargo run --bin iggy-server -- --with-default-root-credentials --fresh
+ ```
+
+3. Run the tests
+
+ `uv`:
+
+ ```bash
+ uv run --no-sync pytest tests/ -v
+ ```
+
+ `pip`:
+
+ ```bash
+ pytest tests/ -v # make sure iggy-server is running and the venv is
activated
+ ```
+
+4. To update the stubs, use
+
+ ```bash
+ cargo run --bin stub_gen
+ ```
+
+5. Before committing, test the pre-commit and pre-push hooks. `prek` only
inspects staged content, so stage your work first:
+
+ ```bash
+ git add -A
+ prek run # runs pre-commit hooks
+ prek run --hook-stage pre-push
+ ```
+
+ These are some of the essential commands prek is running, so it's
recommended to run them manually before running prek / committing / pushing.
This list is not exhaustive and other hook failures are possible.
+
+ ```bash
+ ruff format .
+ ```
+
+ ```bash
+ ruff check --fix .
+ ```
+
+ ```bash
+ cargo fmt --all
+ ```
+
+ ```bash
+ cargo clippy --all-targets --all-features -- -D warnings
+ ```
+
+ ```bash
+ ./scripts/ci/markdownlint.sh --fix # read the diff after applying this,
sometimes it gives unwanted results, e.g. messing up enumerations
Review Comment:
without a path argument the script falls back to `**/*.md`, so `--fix`
rewrites every markdown file in the repo (74 tracked files) - much wider than
the prek hook, which only gets staged filenames. given this line's own warning
about the fixer messing up enumerations, scope it:
`./scripts/ci/markdownlint.sh --fix foreign/python/README.md`. it also only
exists relative to repo root.
##########
foreign/python/README.md:
##########
@@ -38,33 +39,111 @@ pip install apache-iggy
### Local Development
-```bash
-# Start server for testing using docker
-docker compose -f docker-compose.test.yml up --build
-
-# Or use cargo
-cargo run --bin iggy-server -- --with-default-root-credentials --fresh
-
-# Using uv:
-uv sync --all-extras
-uv run maturin develop
-uv run pytest tests/ -v # Run tests (requires iggy-server running)
-
-# Using pip:
-python3 -m venv .venv
-source .venv/bin/activate
-pip install -e ".[all]"
-maturin develop
-pytest tests/ -v # Run tests (requires iggy-server running)
-```
+1. Build a project for development
Review Comment:
prerequisites are understated - the only stated requirement in this file is
python 3.10+, but every path needs a rust toolchain from step 1 onward (this
step literally runs cargo build, and the pip variant compiles the extension
too), plus uv and prek. and since the docker option was dropped, there's no
toolchain-free way to get a server anymore. a short prerequisites line plus a
pointer to the prek install section of the root CONTRIBUTING.md would help.
##########
foreign/python/README.md:
##########
@@ -38,33 +39,111 @@ pip install apache-iggy
### Local Development
-```bash
-# Start server for testing using docker
-docker compose -f docker-compose.test.yml up --build
-
-# Or use cargo
-cargo run --bin iggy-server -- --with-default-root-credentials --fresh
-
-# Using uv:
-uv sync --all-extras
-uv run maturin develop
-uv run pytest tests/ -v # Run tests (requires iggy-server running)
-
-# Using pip:
-python3 -m venv .venv
-source .venv/bin/activate
-pip install -e ".[all]"
-maturin develop
-pytest tests/ -v # Run tests (requires iggy-server running)
-```
+1. Build a project for development
+
+ With `uv`:
+
+ > Create a venv:
Review Comment:
style: blockquote-wrapped code fences are new to this repo - every other
readme (and steps 2-4 below) uses plain fences, and `>` elsewhere means the ASF
disclaimer. collapsing each install path to one fence with `#` comments reads
the same and drops ~40 lines.
##########
foreign/python/README.md:
##########
@@ -38,33 +39,111 @@ pip install apache-iggy
### Local Development
-```bash
-# Start server for testing using docker
-docker compose -f docker-compose.test.yml up --build
-
-# Or use cargo
-cargo run --bin iggy-server -- --with-default-root-credentials --fresh
-
-# Using uv:
-uv sync --all-extras
-uv run maturin develop
-uv run pytest tests/ -v # Run tests (requires iggy-server running)
-
-# Using pip:
-python3 -m venv .venv
-source .venv/bin/activate
-pip install -e ".[all]"
-maturin develop
-pytest tests/ -v # Run tests (requires iggy-server running)
-```
+1. Build a project for development
+
+ With `uv`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > uv venv
+ > ```
+ >
+ > Sync the environment without updating it:
+ >
+ > ```bash
+ > uv sync --frozen --all-extras --no-install-project
+ > ```
+ >
+ > Build the project - this runs cargo build and performs an editable
install:
+ >
+ > ```bash
+ > uv run maturin develop
+ > ```
+
+ With `pip`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > python3 -m venv .venv
+ > ```
+ >
+ > Activate the venv:
+ >
+ > ```bash
+ > source .venv/bin/activate
+ > ```
+ >
+ > Install the dependencies with `pip`:
+ >
+ > ```bash
+ > pip install -e ".[all]"
+ > ```
+
+2. Run the server to be able to run the tests
+
+ ```bash
+ cargo run --bin iggy-server -- --with-default-root-credentials --fresh
+ ```
+
+3. Run the tests
+
+ `uv`:
+
+ ```bash
+ uv run --no-sync pytest tests/ -v
+ ```
+
+ `pip`:
+
+ ```bash
+ pytest tests/ -v # make sure iggy-server is running and the venv is
activated
+ ```
+
+4. To update the stubs, use
+
+ ```bash
+ cargo run --bin stub_gen
+ ```
+
+5. Before committing, test the pre-commit and pre-push hooks. `prek` only
inspects staged content, so stage your work first:
+
+ ```bash
+ git add -A
+ prek run # runs pre-commit hooks
+ prek run --hook-stage pre-push
+ ```
+
+ These are some of the essential commands prek is running, so it's
recommended to run them manually before running prek / committing / pushing.
This list is not exhaustive and other hook failures are possible.
+
+ ```bash
+ ruff format .
Review Comment:
also line 125. bare `ruff` uses whatever is on PATH, and on the uv path the
venv is never activated so it may not exist at all. the versions in play differ
(PATH vs venv vs lock vs the prek pin), and format output changes across
minors, so this can fight the hook. `uv run --no-sync ruff format .` / `uv run
--no-sync ruff check --fix .` matches CI and the synced venv.
##########
foreign/python/README.md:
##########
@@ -38,33 +39,111 @@ pip install apache-iggy
### Local Development
-```bash
-# Start server for testing using docker
-docker compose -f docker-compose.test.yml up --build
-
-# Or use cargo
-cargo run --bin iggy-server -- --with-default-root-credentials --fresh
-
-# Using uv:
-uv sync --all-extras
-uv run maturin develop
-uv run pytest tests/ -v # Run tests (requires iggy-server running)
-
-# Using pip:
-python3 -m venv .venv
-source .venv/bin/activate
-pip install -e ".[all]"
-maturin develop
-pytest tests/ -v # Run tests (requires iggy-server running)
-```
+1. Build a project for development
+
+ With `uv`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > uv venv
+ > ```
+ >
+ > Sync the environment without updating it:
+ >
+ > ```bash
+ > uv sync --frozen --all-extras --no-install-project
+ > ```
+ >
+ > Build the project - this runs cargo build and performs an editable
install:
+ >
+ > ```bash
+ > uv run maturin develop
+ > ```
+
+ With `pip`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > python3 -m venv .venv
+ > ```
+ >
+ > Activate the venv:
+ >
+ > ```bash
+ > source .venv/bin/activate
+ > ```
+ >
+ > Install the dependencies with `pip`:
+ >
+ > ```bash
+ > pip install -e ".[all]"
+ > ```
+
+2. Run the server to be able to run the tests
+
+ ```bash
+ cargo run --bin iggy-server -- --with-default-root-credentials --fresh
+ ```
+
+3. Run the tests
+
+ `uv`:
+
+ ```bash
+ uv run --no-sync pytest tests/ -v
+ ```
+
+ `pip`:
+
+ ```bash
+ pytest tests/ -v # make sure iggy-server is running and the venv is
activated
+ ```
+
+4. To update the stubs, use
+
+ ```bash
+ cargo run --bin stub_gen
Review Comment:
two things: the command only resolves from `foreign/python` (the crate is
not in the root workspace), and it's worth saying when to run it - only after
changing the pyo3 surface; nothing in CI checks stub freshness, so
unconditional regen just invites `.pyi` churn. separate but related:
`stub_gen.rs` builds its license-prepend path from `file!()`, which resolves
against the runtime cwd - run this from any subdirectory of `foreign/python`
and it rewrites the tracked `apache_iggy.pyi` without the apache header before
failing with a bare ENOENT. the CI license gate catches it, but
`Path::new(env!("CARGO_MANIFEST_DIR"))` in that binary would make step 4
cwd-independent. happy to see that land separately since the code is outside
this diff.
##########
foreign/python/README.md:
##########
@@ -38,33 +39,111 @@ pip install apache-iggy
### Local Development
-```bash
-# Start server for testing using docker
-docker compose -f docker-compose.test.yml up --build
-
-# Or use cargo
-cargo run --bin iggy-server -- --with-default-root-credentials --fresh
-
-# Using uv:
-uv sync --all-extras
-uv run maturin develop
-uv run pytest tests/ -v # Run tests (requires iggy-server running)
-
-# Using pip:
-python3 -m venv .venv
-source .venv/bin/activate
-pip install -e ".[all]"
-maturin develop
-pytest tests/ -v # Run tests (requires iggy-server running)
-```
+1. Build a project for development
+
+ With `uv`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > uv venv
+ > ```
+ >
+ > Sync the environment without updating it:
+ >
+ > ```bash
+ > uv sync --frozen --all-extras --no-install-project
+ > ```
+ >
+ > Build the project - this runs cargo build and performs an editable
install:
+ >
+ > ```bash
+ > uv run maturin develop
+ > ```
+
+ With `pip`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > python3 -m venv .venv
+ > ```
+ >
+ > Activate the venv:
+ >
+ > ```bash
+ > source .venv/bin/activate
+ > ```
+ >
+ > Install the dependencies with `pip`:
+ >
+ > ```bash
+ > pip install -e ".[all]"
+ > ```
+
+2. Run the server to be able to run the tests
+
+ ```bash
+ cargo run --bin iggy-server -- --with-default-root-credentials --fresh
+ ```
+
+3. Run the tests
+
+ `uv`:
+
+ ```bash
+ uv run --no-sync pytest tests/ -v
+ ```
+
+ `pip`:
+
+ ```bash
+ pytest tests/ -v # make sure iggy-server is running and the venv is
activated
+ ```
+
+4. To update the stubs, use
+
+ ```bash
+ cargo run --bin stub_gen
+ ```
+
+5. Before committing, test the pre-commit and pre-push hooks. `prek` only
inspects staged content, so stage your work first:
+
+ ```bash
+ git add -A
Review Comment:
this sequence wedges when any fixer hook modifies a file: fixes land in the
worktree, the index keeps the unfixed blob, `prek run` exits 1, and the
pre-push run just stashes the fix and re-checks the stale index (`Hook changes
conflicted with the saved unstaged changes. Reverting the hook changes`) - zero
progress until you re-stage. add a note to re-run `git add -A` after any run
where hooks changed files. (`prek run --all-files` is not a substitute - it
reads the worktree and passes while the staged blob is still broken, and it
feeds the whole repo to the `typos -w` fixer.)
##########
foreign/python/README.md:
##########
@@ -38,33 +39,111 @@ pip install apache-iggy
### Local Development
-```bash
-# Start server for testing using docker
-docker compose -f docker-compose.test.yml up --build
-
-# Or use cargo
-cargo run --bin iggy-server -- --with-default-root-credentials --fresh
-
-# Using uv:
-uv sync --all-extras
-uv run maturin develop
-uv run pytest tests/ -v # Run tests (requires iggy-server running)
-
-# Using pip:
-python3 -m venv .venv
-source .venv/bin/activate
-pip install -e ".[all]"
-maturin develop
-pytest tests/ -v # Run tests (requires iggy-server running)
-```
+1. Build a project for development
+
+ With `uv`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > uv venv
+ > ```
+ >
+ > Sync the environment without updating it:
+ >
+ > ```bash
+ > uv sync --frozen --all-extras --no-install-project
+ > ```
+ >
+ > Build the project - this runs cargo build and performs an editable
install:
+ >
+ > ```bash
+ > uv run maturin develop
+ > ```
+
+ With `pip`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > python3 -m venv .venv
+ > ```
+ >
+ > Activate the venv:
+ >
+ > ```bash
+ > source .venv/bin/activate
+ > ```
+ >
+ > Install the dependencies with `pip`:
+ >
+ > ```bash
+ > pip install -e ".[all]"
+ > ```
+
+2. Run the server to be able to run the tests
+
+ ```bash
+ cargo run --bin iggy-server -- --with-default-root-credentials --fresh
+ ```
+
+3. Run the tests
+
+ `uv`:
+
+ ```bash
+ uv run --no-sync pytest tests/ -v
+ ```
+
+ `pip`:
+
+ ```bash
+ pytest tests/ -v # make sure iggy-server is running and the venv is
activated
+ ```
+
+4. To update the stubs, use
+
+ ```bash
+ cargo run --bin stub_gen
+ ```
+
+5. Before committing, test the pre-commit and pre-push hooks. `prek` only
inspects staged content, so stage your work first:
+
+ ```bash
+ git add -A
+ prek run # runs pre-commit hooks
+ prek run --hook-stage pre-push
+ ```
+
+ These are some of the essential commands prek is running, so it's
recommended to run them manually before running prek / committing / pushing.
This list is not exhaustive and other hook failures are possible.
+
+ ```bash
+ ruff format .
+ ```
+
+ ```bash
+ ruff check --fix .
+ ```
+
+ ```bash
+ cargo fmt --all
+ ```
+
+ ```bash
+ cargo clippy --all-targets --all-features -- -D warnings
+ ```
+
+ ```bash
+ ./scripts/ci/markdownlint.sh --fix # read the diff after applying this,
sometimes it gives unwanted results, e.g. messing up enumerations
+ ```
## Examples
Refer to the
[examples/python/](https://github.com/apache/iggy/tree/master/examples/python)
directory for usage examples.
## Contributing
-See
[CONTRIBUTING.md](https://github.com/apache/iggy/blob/master/foreign/python/CONTRIBUTING.md)
for development setup and guidelines.
+See
[CONTRIBUTING.md](https://github.com/apache/iggy/blob/master/CONTRIBUTING.md)
for development setup and guidelines.
Review Comment:
the retarget itself is right (`foreign/python/CONTRIBUTING.md` doesn't
exist, so the old link was a 404), but the root CONTRIBUTING.md delegates
language-specific setup back to this very readme, so "for development setup"
now sends the reader in a circle. "for contribution guidelines" fits better.
##########
foreign/python/README.md:
##########
@@ -38,33 +39,111 @@ pip install apache-iggy
### Local Development
-```bash
-# Start server for testing using docker
-docker compose -f docker-compose.test.yml up --build
-
-# Or use cargo
-cargo run --bin iggy-server -- --with-default-root-credentials --fresh
-
-# Using uv:
-uv sync --all-extras
-uv run maturin develop
-uv run pytest tests/ -v # Run tests (requires iggy-server running)
-
-# Using pip:
-python3 -m venv .venv
-source .venv/bin/activate
-pip install -e ".[all]"
-maturin develop
-pytest tests/ -v # Run tests (requires iggy-server running)
-```
+1. Build a project for development
+
+ With `uv`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > uv venv
+ > ```
+ >
+ > Sync the environment without updating it:
+ >
+ > ```bash
+ > uv sync --frozen --all-extras --no-install-project
+ > ```
+ >
+ > Build the project - this runs cargo build and performs an editable
install:
+ >
+ > ```bash
+ > uv run maturin develop
+ > ```
+
+ With `pip`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > python3 -m venv .venv
+ > ```
+ >
+ > Activate the venv:
+ >
+ > ```bash
+ > source .venv/bin/activate
+ > ```
+ >
+ > Install the dependencies with `pip`:
+ >
+ > ```bash
+ > pip install -e ".[all]"
+ > ```
+
+2. Run the server to be able to run the tests
+
+ ```bash
+ cargo run --bin iggy-server -- --with-default-root-credentials --fresh
+ ```
+
+3. Run the tests
+
+ `uv`:
+
+ ```bash
+ uv run --no-sync pytest tests/ -v
+ ```
+
+ `pip`:
+
+ ```bash
+ pytest tests/ -v # make sure iggy-server is running and the venv is
activated
+ ```
+
+4. To update the stubs, use
+
+ ```bash
+ cargo run --bin stub_gen
+ ```
+
+5. Before committing, test the pre-commit and pre-push hooks. `prek` only
inspects staged content, so stage your work first:
+
+ ```bash
+ git add -A
+ prek run # runs pre-commit hooks
+ prek run --hook-stage pre-push
+ ```
+
+ These are some of the essential commands prek is running, so it's
recommended to run them manually before running prek / committing / pushing.
This list is not exhaustive and other hook failures are possible.
Review Comment:
double space in "before running". this line is also 212 chars - worth
wrapping.
##########
foreign/python/README.md:
##########
@@ -38,33 +39,111 @@ pip install apache-iggy
### Local Development
-```bash
-# Start server for testing using docker
-docker compose -f docker-compose.test.yml up --build
-
-# Or use cargo
-cargo run --bin iggy-server -- --with-default-root-credentials --fresh
-
-# Using uv:
-uv sync --all-extras
-uv run maturin develop
-uv run pytest tests/ -v # Run tests (requires iggy-server running)
-
-# Using pip:
-python3 -m venv .venv
-source .venv/bin/activate
-pip install -e ".[all]"
-maturin develop
-pytest tests/ -v # Run tests (requires iggy-server running)
-```
+1. Build a project for development
+
+ With `uv`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > uv venv
+ > ```
+ >
+ > Sync the environment without updating it:
+ >
+ > ```bash
+ > uv sync --frozen --all-extras --no-install-project
+ > ```
+ >
+ > Build the project - this runs cargo build and performs an editable
install:
+ >
+ > ```bash
+ > uv run maturin develop
Review Comment:
missing `--no-sync`. bare `uv run` implicitly syncs, which builds and
installs the project via the pep517 backend (release profile) right before
`maturin develop` builds it again in debug - two full cargo builds per
iteration, one thrown away. it also undoes the `--no-install-project` added on
line 55, so that flag currently buys nothing. line 95 already does this
correctly, and CI uses `uv run --no-sync maturin develop` in
`.github/actions/python-maturin/pre-merge/action.yml`.
##########
foreign/python/README.md:
##########
@@ -38,33 +39,111 @@ pip install apache-iggy
### Local Development
-```bash
-# Start server for testing using docker
-docker compose -f docker-compose.test.yml up --build
-
-# Or use cargo
-cargo run --bin iggy-server -- --with-default-root-credentials --fresh
-
-# Using uv:
-uv sync --all-extras
-uv run maturin develop
-uv run pytest tests/ -v # Run tests (requires iggy-server running)
-
-# Using pip:
-python3 -m venv .venv
-source .venv/bin/activate
-pip install -e ".[all]"
-maturin develop
-pytest tests/ -v # Run tests (requires iggy-server running)
-```
+1. Build a project for development
+
+ With `uv`:
+
+ > Create a venv:
+ >
+ > ```bash
+ > uv venv
+ > ```
+ >
+ > Sync the environment without updating it:
+ >
+ > ```bash
+ > uv sync --frozen --all-extras --no-install-project
+ > ```
+ >
+ > Build the project - this runs cargo build and performs an editable
install:
Review Comment:
"editable install" oversells it - there's no python source to edit; `maturin
develop` copies a compiled module into the venv, so every rust change needs a
re-run. also worth noting the two paths land on different build profiles:
`maturin develop` builds debug while the pip path's pep517 build is release,
and with the 30s per-test timeout the debug extension is the more timeout-prone
one. suggestion: "builds the rust extension into the venv (debug profile) -
re-run after any rust change".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
