fix(proto-gen): fix buf plugin path replacement, GIT_TAG, and generated_dir#135
fix(proto-gen): fix buf plugin path replacement, GIT_TAG, and generated_dir#135
Conversation
…ed_dir - Replace silent str.replace() with re.sub() so the protoc-gen-connect-python path in buf.gen.yaml is correctly updated regardless of whether it contains a relative path, absolute path, or underscore/hyphen naming variant - Make GIT_TAG overridable via --tag CLI argument; default bumped to service/v0.12.0 - Fix generated_dir to point at src/otdf_python_proto/ (the actual buf output location) instead of the non-existent generated/ directory - Remove unreachable return False after finally block in copy_opentdf_proto_files() - Implement setup_connect_rpc.py (was empty in source control) Fixes #133
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the proto generation workflow by introducing a --tag argument to specify the OpenTDF platform version, updating the default version to v0.12.0, and redirecting generated output to a new source directory. It also adds a setup script for Connect RPC dependencies. Review feedback recommends refining the regex for plugin path replacement to handle cases without path prefixes, improving the robustness of manual argument parsing for the tag flag, and utilizing uv sync instead of uv add to avoid unintended modifications to the project's configuration files.
- 25 unit tests covering all bugs from #133 (fixed) and #134 (open); tests for #134 intentionally fail to document the known issue - Add pytest to otdf-python-proto dev dependencies - Add proto-unit-tests CI job to test-suite.yaml, running in parallel after lint-check with working-directory: otdf-python-proto - Fix regex from \S+ to \S* so it also matches bare plugin name (no path prefix)
|
/gemini review |
Remove unused imports (call, pytest) and sort import block.
There was a problem hiding this comment.
Code Review
This pull request adds unit tests for the proto generation script, updates the default OpenTDF platform version to v0.12.0, and introduces a --tag command-line argument. It also relocates generated code to the src directory and provides a setup script for dependencies. The review feedback recommends using the argparse module for more robust argument parsing and employing a lambda function within re.sub to safely handle absolute file paths and variations in whitespace.
… files create_init_files() was only walking one level deep, leaving nested packages like authorization/v2/ and policy/attributes/ without __init__.py files, causing ImportError on a clean generation run. Fixes #134
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the proto generation script by implementing recursive __init__.py creation, adding support for custom Git tags, and updating the output directory structure. Additionally, it introduces a setup script for Connect RPC and a new test suite. Feedback suggests migrating to argparse for more reliable CLI argument parsing and using a lambda in re.sub to ensure literal string replacement for paths, preventing potential issues on Windows.
- Add argparse import and replace fragile manual sys.argv parsing with argparse in generate_connect_proto.py - Use lambda in re.sub replacement to avoid backslash interpretation in paths on Windows - Relax regex to use \s+ and \S* so plugin path prefix is optional - Replace `uv add` with `uv sync` in setup_connect_rpc.py to avoid modifying pyproject.toml/uv.lock for all users
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the proto generation script by adding command-line support for git tags and forced downloads, implementing recursive init.py creation, and using regex for robust plugin path replacement in buf.gen.yaml. It also introduces a setup script for dependencies and a comprehensive unit test suite. A review comment suggests using the more idiomatic and safer shutil.rmtree instead of a subprocess call for removing temporary directories.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces unit testing with pytest, adds a setup script for Connect RPC dependencies, and improves the proto generation script by implementing recursive init.py creation and argparse for configuration. Feedback suggests simplifying a regex replacement in the generation script and refactoring argument parsing tests to ensure they validate the actual argparse implementation rather than a custom mock.
Removes shell-out to rm -rf for temp directory cleanup, using the stdlib shutil.rmtree instead. Updates tests to assert the directory no longer exists rather than inspecting subprocess calls.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the proto generation script to use argparse for command-line arguments, improves file operations with shutil, and enhances the robustness of buf.gen.yaml updates using regex. It also updates the output directory structure and ensures recursive init.py creation. A new setup script and unit tests were added. Feedback indicated that the argument parsing tests should be updated to test the actual ArgumentParser configuration from the production code instead of a manual implementation to ensure proper validation.
Replace the private _parse_tag helper (which tested a hand-rolled parser that no longer exists) with _get_parser(), which replicates the ArgumentParser setup from main() and calls parse_args() directly.
…tory - Replace all references to generated/ with src/otdf_python_proto/, which is where buf generate now writes files - Drop the mkdir -p generated/ call (no longer needed) - Switch uv sync --dev (deprecated flag) to plain uv sync - Replace silent uv add fallback with a hard error pointing to setup_connect_rpc.py, so missing dependencies are caught early
- DEVELOPING.md: replace reference to non-existent PROTOBUF_SETUP.md with the correct CONNECT_RPC.md - otdf-python-proto/README.md: replace `uv add connect-python[compiler]` troubleshooting step with `uv run python scripts/setup_connect_rpc.py`, which is the correct setup path now that setup_connect_rpc.py exists
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Connect RPC generation process, moving generated files to the src directory and enhancing the generation scripts with CLI argument support for Git tags and forced downloads. It also introduces a dedicated setup script and a comprehensive test suite. Feedback identifies a redundancy in dependency synchronization within the build script and a naming inconsistency in the new unit tests where the test name contradicts the assertion logic.
| # Install dependencies | ||
| echo "Installing/updating dependencies..." | ||
| cd "$PROTO_GEN_DIR" | ||
| uv sync --dev | ||
| uv sync |
There was a problem hiding this comment.
This script runs uv sync, and then if connectrpc is not found, it advises the user to run setup_connect_rpc.py, which also just runs uv sync. This is redundant and potentially confusing.
To improve clarity, I suggest removing the uv sync from this build script. The script should focus on generation and assume dependencies are already installed. The existing check for connect-python will then correctly guide the user to run the setup script if needed.
| # Install dependencies | |
| echo "Installing/updating dependencies..." | |
| cd "$PROTO_GEN_DIR" | |
| uv sync --dev | |
| uv sync | |
| cd "$PROTO_GEN_DIR" |
| def test_default_tag_is_not_old_hardcode(self, tmp_path): | ||
| """The default must no longer be the stale service/v0.7.2.""" |
There was a problem hiding this comment.
The test name and docstring are misleading. The test asserts that the default tag is service/v0.7.2, but the name and docstring suggest it should not be. This can be confusing for future maintainers. Please update them to accurately reflect the test's purpose.
| def test_default_tag_is_not_old_hardcode(self, tmp_path): | |
| """The default must no longer be the stale service/v0.7.2.""" | |
| def test_default_tag_is_used_when_no_tag_is_provided(self, tmp_path): | |
| """The default tag must be used when no git_tag is provided.""" |
Improve the connect-python missing error message to accurately state the dependency is absent (not just unsynced), and rename the test to reflect what it actually asserts.
1 participant
Fixes two bugs in the proto generation script:
Bug 1 (fixes #133): The script wasn't working reliably — it would silently skip updating a config file it needed to change, always download from the same old version tag no matter what you asked for, and put some helper files in the wrong folder.
Bug 2 (fixes #134): When setting up Python package folders after generation, the script only went one folder deep. Nested folders (like
authorization/v2/) were missing the__init__.pyfile Python needs to treat them as packages, causing import errors.Also adds unit tests for both bugs and a CI job to run them.