Copilot commented on code in PR #299:
URL:
https://github.com/apache/pulsar-client-python/pull/299#discussion_r3217174372
##########
tests/test_schema_pb2.py:
##########
@@ -0,0 +1,40 @@
+# -*- coding: utf-8 -*-
+# Generated by the protocol buffer compiler. DO NOT EDIT!
+# NO CHECKED-IN PROTOBUF GENCODE
+# source: test_schema.proto
+# Protobuf Python Version: 6.32.0
+"""Generated protocol buffer code."""
+from google.protobuf import descriptor as _descriptor
+from google.protobuf import descriptor_pool as _descriptor_pool
+from google.protobuf import runtime_version as _runtime_version
+from google.protobuf import symbol_database as _symbol_database
+from google.protobuf.internal import builder as _builder
+_runtime_version.ValidateProtobufRuntimeVersion(
+ _runtime_version.Domain.PUBLIC,
+ 6,
+ 32,
+ 0,
+ '',
+ 'test_schema.proto'
+)
Review Comment:
This checked-in generated module hard-requires a protobuf runtime matching
the codegen version via `_runtime_version.ValidateProtobufRuntimeVersion(...,
6, 32, 0, ...)`. That will raise on import when the environment has an older
protobuf runtime, making the test suite fragile (and inconsistent with this
repo’s declared protobuf minimum). Consider regenerating with the minimum
supported protobuf/protoc, or removing the runtime version gate by generating
with a compatible protoc version and re-checking in the output.
##########
tests/test_schema_pb2.py:
##########
@@ -0,0 +1,40 @@
+# -*- coding: utf-8 -*-
+# Generated by the protocol buffer compiler. DO NOT EDIT!
+# NO CHECKED-IN PROTOBUF GENCODE
+# source: test_schema.proto
+# Protobuf Python Version: 6.32.0
+"""Generated protocol buffer code."""
Review Comment:
This new Python source file does not include the standard ASF license header
that is present at the top of other Python sources in this repository
(including other test files). If this file must be committed, add the ASF
header (even if it remains generated), or switch to generating it during the
test run so the repository doesn’t carry non-compliant generated artifacts.
##########
tests/schema_test.py:
##########
@@ -30,6 +33,10 @@
import json
from fastavro.schema import load_schema
+# Make generated protobuf test classes importable
+sys.path.insert(0, os.path.dirname(__file__))
+from test_schema_pb2 import TestMessage, TestMessageWithNested, TestInner
Review Comment:
Mutating `sys.path` at import time can leak into the rest of the test
process and change how other modules are resolved (especially in a large test
file like this). Prefer importing the generated module without global path
changes (e.g., load it via `importlib` from an explicit file path, or make
`tests/` a package and use a package import) so test isolation isn’t affected.
##########
setup.py:
##########
@@ -76,14 +76,21 @@ def build_extension(self, ext):
extras_require = {}
+# protobuf schema dependencies
+extras_require["protobuf"] = sorted(
+ {
+ "protobuf>=3.6.1",
Review Comment:
The declared extra `protobuf` allows very old runtimes (`protobuf>=3.6.1`),
but the committed test gencode (`tests/test_schema_pb2.py`) enforces protobuf
runtime 6.32.0 via `ValidateProtobufRuntimeVersion`. Either bump the minimum
version in this extra to match what the tests (and generated code) require, or
regenerate the test gencode with a protoc/runtime compatible with the supported
minimum to avoid surprising import failures.
--
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]
