#195 support for etl std byte as byte type#231
Conversation
* introducing lrpc::byte iso LRPC_BYTE_TYPE * Added tests for encoding/decoding etl::byte and std::byte in EtlRwExtensions.hpp. By virtue of etl::byte and std::byte being an enum class, this was already supported, but there were no tests
* Added test file for etl::byte
* Added byte_type settings
* Fix default byte type in tests
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds configurable C++ byte types via a new lrpc::byte alias and byte_type setting, implements a DefinitionLoader with multi-document YAML overlay merge semantics, updates codegen/export/runtime to be byte-type aware, and expands docs/tests (including a comprehensive overlay-merge test suite). Changes
Sequence Diagram(s)sequenceDiagram
participant User as Caller
participant Loader as DefinitionLoader
participant Merge as merge_definition
participant Validator as jsonschema
participant Analyzer as SemanticAnalyzer
participant Def as LrpcDef
User->>Loader: __init__(base_definition, include_meta_def?)
Loader->>Loader: load base (str|Path|TextIO)
alt include_meta_def
Loader->>Loader: load meta definition
Loader->>Merge: merge_definition(current, meta)
end
loop overlays
User->>Loader: add_overlay(overlay_source)
Loader->>Loader: load all YAML docs
Loader->>Merge: merge_definition(current, doc)
end
User->>Loader: lrpc_def()
Loader->>Validator: jsonschema.validate(merged)
Validator-->>Loader: valid
Loader->>Analyzer: SemanticAnalyzer.analyze(merged)
Analyzer-->>Loader: analysis OK
Loader->>Def: LrpcDef(merged) --deep copy & init
Def-->>User: LrpcDef instance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Test Results 2 files ± 0 2 suites ±0 20s ⏱️ -5s Results for commit 9eb75bd. ± Comparison against base commit f23c6a2. This pull request removes 91 and adds 71 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
* Add news fragment * Add support for overlays
* Add mergedeep dependency
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
* Raw definition dict is immutable * Added overlay_merge * Using TextIOWrapper iso TextIO
* Removed dependency on mergedeep * lrpcg has single option for all types of overlays * Some refactoring
* Removed duplicate test * Added new, missing tests * Fixed bug
* Some refactoring
* Some refactoring and completion of functionality
…github.com/tzijnge/LotusRpc into #195-support-for-etl-std-byte-as-byte-type
* Fix and test byte_type
* All bytearray tests are now a separate executable to prevent type collisions
* Added lrpcg merge * load_definition.py has support for multiple YAML documents in one overlay. * Overlays and meta service are loaded with normal yaml.safe_load functions as functions_before_streams is not applicable for them * functions_before_streams is now optional for LrpcService
* Update documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lrpc/codegen/meta_service_file_writer.py (1)
13-13: 🧹 Nitpick | 🔵 TrivialAlign span and take types to use consistent byte representation.
The generated code declares
dataaslrpc::span<const uint8_t>(line 13) but callstake<const lrpc::byte>()(line 22). SinceCompressedDefinitionis typed aslrpc::array<uint8_t, ...>andlrpc::byteis a configurable type ($byte_type), this creates a type mismatch whenlrpc::bytediffers fromuint8_t(e.g., when configured asstd::byte).Update
CompressedDefinitionto uselrpc::byteinstead ofuint8_tfor consistency, or verify the type conversion is safe across all supported byte type configurations.tests/python/test_lrpc_client.py (1)
436-468: 🧹 Nitpick | 🔵 TrivialAvoid hard-coded definition-hash literals in mismatch assertions.
Pinning
"8d9f42cd4dba8846..."will cause avoidable test churn whenever the YAML changes. Compute the client-side hash prefix fromlrpc_def.definition_hash()instead.Proposed refactor
+ client_hash_prefix = f"{(lrpc_def.definition_hash() or '')[:16]}..." assert "Server mismatch detected. Details client vs server:" in caplog.messages assert f"LotusRPC version: {lrpc_version} vs {lrpc_version}" in caplog.messages assert "Definition version: [disabled] vs [disabled]" in caplog.messages - assert "Definition hash: 8d9f42cd4dba8846... vs [wrong hash]..." in caplog.messages + assert f"Definition hash: {client_hash_prefix} vs [wrong hash]..." in caplog.messages ... - assert "Definition hash: 8d9f42cd4dba8846... vs 8d9f42cd4dba8846..." in caplog.messages + assert f"Definition hash: {client_hash_prefix} vs {client_hash_prefix}" in caplog.messages ... - assert "Definition hash: 8d9f42cd4dba8846... vs 8d9f42cd4dba8846..." in caplog.messages + assert f"Definition hash: {client_hash_prefix} vs {client_hash_prefix}" in caplog.messages
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08be6daf-043b-4ada-8878-fb9033351541
📒 Files selected for processing (57)
changes/195.feature.mddocs/index.mddocs/reference.mddocs/tools.mdsrc/lrpc/client/lrpc_client.pysrc/lrpc/codegen/constants.pysrc/lrpc/codegen/meta_service_file_writer.pysrc/lrpc/core/definition.pysrc/lrpc/core/service.pysrc/lrpc/core/settings.pysrc/lrpc/resources/cpp/EtlRwExtensions.hppsrc/lrpc/resources/cpp/LrpcTypes.hppsrc/lrpc/resources/cpp/export.pysrc/lrpc/resources/meta/meta.lrpc.yamlsrc/lrpc/schema/lotusrpc-schema.jsonsrc/lrpc/tools/lrpcc/lrpcc.pysrc/lrpc/tools/lrpcg/lrpcg.pysrc/lrpc/utils/__init__.pysrc/lrpc/utils/load_definition.pysrc/lrpc/utils/overlay_merge.pytests/cpp/CMakeLists.txttests/cpp/TestBytearray.template.cpptests/cpp/TestBytearray_char.cpptests/cpp/TestBytearray_char8_t.cpptests/cpp/TestBytearray_etl_byte.cpptests/cpp/TestBytearray_int8_t.cpptests/cpp/TestBytearray_signed_char.cpptests/cpp/TestBytearray_std_byte.cpptests/cpp/TestBytearray_uint8_t.cpptests/cpp/TestBytearray_unsigned_char.cpptests/cpp/TestEtlRwExtensions.cpptests/cpp/TestRetrieveDefinition.cpptests/cpp/TestServer4.cpptests/cpp/TestUtils.hpptests/embedded_definition/embedded_definition.pytests/python/test_lrpc_client.pytests/python/test_lrpc_decode.pytests/python/test_lrpc_def.pytests/python/test_lrpc_encode.pytests/python/test_lrpc_encode_decode.pytests/python/test_lrpc_service.pytests/python/test_meta_service.pytests/python/test_meta_service_file_writer.pytests/python/test_overlay_merge.pytests/python/test_rpc_settings.pytests/python/test_schema.pytests/python/utilities.pytests/testdata/TestBytearray_char.lrpc.yamltests/testdata/TestBytearray_char8_t.lrpc.yamltests/testdata/TestBytearray_etl_byte.lrpc.yamltests/testdata/TestBytearray_int8_t.lrpc.yamltests/testdata/TestBytearray_signed_char.lrpc.yamltests/testdata/TestBytearray_std_byte.lrpc.yamltests/testdata/TestBytearray_uint8_t.lrpc.yamltests/testdata/TestBytearray_unsigned_char.lrpc.yamltests/testdata/TestRetrieveDefinition.lrpc.yamltests/testdata/test_lrpc_encode_decode.lrpc.yaml
💤 Files with no reviewable changes (1)
- tests/cpp/TestUtils.hpp
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
docs/tools.md (1)
18-18:⚠️ Potential issue | 🟡 MinorFix the anchor link to match the actual heading.
The link
reference.md#definition-overlaysuses singular "definition", but the heading inreference.md(line 243) is "Definitions overlays" (plural), which generates the anchor#definitions-overlays.📝 Proposed fix
-Definition overlays can be specified with one or more `-ov` options. A single overlay may contain multiple [YAML documents](https://yaml.org/spec/1.2.2/#91-documents). For more information about definition overlays see [Definition overlays](reference.md#definition-overlays). +Definition overlays can be specified with one or more `-ov` options. A single overlay may contain multiple [YAML documents](https://yaml.org/spec/1.2.2/#91-documents). For more information about definition overlays see [Definition overlays](reference.md#definitions-overlays).docs/reference.md (1)
257-267: 🧹 Nitpick | 🔵 TrivialClarify
addbehavior for existing named composites.The merge strategy table states that
addrequires "Item does not exist in base", but Example 1 showsmerge_strategy: addbeing used to add a parameter to an existing functionDoWork. This works because the strategy is inherited to child properties, but the documentation could be clearer thataddon a named composite (like a function) will merge into the existing item rather than requiring it to be absent.Consider adding a clarifying note:
When
addis applied to a named composite that already exists in the base, the strategy is inherited to child properties, allowing new child items to be added.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a1df3ad5-4184-4a85-b85d-b7110476153c
📒 Files selected for processing (8)
docs/binary.mddocs/reference.mddocs/tools.mdsrc/lrpc/resources/cpp/export.pysrc/lrpc/utils/load_definition.pytests/cpp/CMakeLists.txttests/cpp/TestEtlRwExtensions.cpptests/python/test_lrpc_client.py
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bf138c31-86e8-4952-ac68-44807ca76327
📒 Files selected for processing (3)
docs/reference.mdsrc/lrpc/resources/cpp/export.pytests/cpp/CMakeLists.txt
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/reference.md (1)
257-262:⚠️ Potential issue | 🟠 MajorClarify the
addprecondition to resolve ambiguity.The precondition table states that
addrequires "Item does not exist in base", but line 265 contradicts this by stating "Adding to a named composite merges properties into an existing item if the name matches." This creates confusion about whenaddcan be used.Example 1 demonstrates adding to an existing function
DoWork, which violates the stated precondition but is explicitly allowed per line 265.The precondition should distinguish between:
- Basic items in lists: Must not exist in base
- Named composites (services, functions, structs, etc.): Can exist; properties will merge into the existing item
📝 Suggested clarification
| Strategy | Behavior | Precondition | |----------|-----------------------------|-----------------------------| -| add | Add a property to base | Item does not exist in base | +| add | Add a property to base | Named composites: merges into existing item by name. Basic items: must not exist in base | | remove | Remove a property from base | Item exists in base | | replace | Replace a property in base | Item exists in base |Alternatively, add a footnote or expand the description to clarify this distinction more explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 542cd8cd-665b-4261-894d-2513eadfeccb
📒 Files selected for processing (2)
docs/reference.mdtests/cpp/CMakeLists.txt
|
1 participant
❇️ @coderabbitai
Description
Fixes issue #195
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests
Type