Add builder registration feature enable#3
Conversation
MonikaKumari26
force-pushed
the
builder-registration-feature
branch
3 times, most recently
from
345953a to
4a798ee
Compare
MonikaKumari26
force-pushed
the
builder-registration-feature
branch
from
4a798ee to
a56dee3
Compare
…accessor and registration handle added
bharatGoswami8
left a comment
bharatGoswami8
left a comment
There was a problem hiding this comment.
Some General Suggestion and I feel we can optimize builder pattern, as we are constructing same type as build type to build the type, can't we avoid this using trait based approach ?
| pub mod sovd { | ||
| /// cf. ISO 17978-3:2025 Section 7.9.1, Table 70 | ||
| #[derive(Clone, Debug, PartialEq)] | ||
| pub enum DataCategory { |
There was a problem hiding this comment.
how moving this into common file ?
There was a problem hiding this comment.
DataCategory is to classify the SOVD data resources that's why it is here. The common module is intentionally kept to types that are shared across both SOVD and UDS.
| pub mod uds { | ||
| pub use common::uds::*; | ||
| pub use uds::*; | ||
| pub use uds_adapters::*; | ||
| pub use registration::*; | ||
| } |
There was a problem hiding this comment.
| } | |
| pub mod uds { | |
| pub use crate::{ | |
| common::uds::*, | |
| uds_adapters::*, | |
| registration::*, | |
| }; | |
| } |
There was a problem hiding this comment.
Appreciate the style suggestion !! However, pub use crate:: cannot be used here, check once the BUILD file.
| pub type KeyValueAttributes = ::common::KeyValueAttributes; | ||
| pub type ReplyMessageEncoding = ::common::ReplyMessageEncoding; | ||
| pub type ReplyMessagePayload = ::common::ReplyMessagePayload; | ||
| pub type RequestMessagePayload = ::common::RequestMessagePayload; | ||
| pub type DiagnosticReply = ::common::DiagnosticReply; | ||
| pub type ByteVector = ::common::ByteVector; | ||
| pub type JsonSchema = ::common::JsonSchema; | ||
| pub type Result<T> = ::common::Result<T>; | ||
| pub type ErrorCode = ::common::ErrorCode; | ||
| pub type Error = ::common::Error; |
There was a problem hiding this comment.
why this alias is not in common ?
| * SPDX-License-Identifier: Apache-2.0 | ||
| ********************************************************************************/ | ||
|
|
||
| use common::Result as DiagResult; |
There was a problem hiding this comment.
This is a private module local alias, nothing to add in common
| } | ||
|
|
||
| /// cf. ISO 17978-3:2025 Section 7.14.7, Table 186 | ||
| pub type ExecutionId = String; |
There was a problem hiding this comment.
I feel this type aliasing should be in some other file kike common.
There was a problem hiding this comment.
concept specific to SOVD operation execution lifecycle..
| } | ||
|
|
||
| /// cf. ISO 17978-3:2025 Section 7.14.7, Table 186 | ||
| pub type ExecutionId = String; |
There was a problem hiding this comment.
why not new type pub struct ExecutionId(String)
There was a problem hiding this comment.
this comment should be relevant to this PR : eclipse-score#2
| #[must_use] | ||
| pub fn default() -> Self { | ||
| Self { | ||
| last_executed_capability: "n/a".to_string(), |
There was a problem hiding this comment.
how about using Option<String> ?
| /// Kind type for events delivered to an operation's execution control loop. | ||
| /// Maps to the SOVD capability model (cf. ISO 17978-3:2025 Section 7.14.9). | ||
| #[derive(Clone)] | ||
| pub enum ExecutionEventKind { |
There was a problem hiding this comment.
We can create one enum module and keep it there ?
There was a problem hiding this comment.
Create one trait Builder and implement for the type DiagnosticServicesCollectionBuilder
pub trait Builder<Output> {
fn build(self) -> Result<Output>;
}
impl Builder<DiagnosticServicesCollectionBuilder> for DiagnosticServicesCollectionBuilder
{
...
}
with this you can make Builder trait as a super trait for other trait and create dependency.
There was a problem hiding this comment.
Thanks for the suggestion... Builder trait would only be beneficial if something needs to accept builders polymorphically, example a function taking T: Builder. In the design there is no such consumer. Here the ServiceRegistrar receives the finished collection, not the builder, so there is no call site where a Builder bound is needed as of now..
| /// Entity-agnostic builder for UDS diagnostic services (DIDs and routines) | ||
| #[derive(Default)] | ||
| pub struct UdsServicesCollectionBuilder { | ||
| read_dids: IndexMap<String, Box<dyn ReadDataByIdentifier + Send + 'static>>, |
There was a problem hiding this comment.
All the above read field type is Sync bound, why not this ?
A trait-based approach would add a layer of abstraction without any performance gain, because the builder must hold heterogeneous instances which are incompatible with typestate patterns and also the builder is always constructed, populated and consumed in a single chain.. Happy to discuss further if you had a specific shape in mind.. |
|
3 participants
This PR adds the builder registration feature to enable dynamic builder registration in the diagnostics library. Implemented builder registration mechanism and Included comprehensive tests