-
Notifications
You must be signed in to change notification settings - Fork 21
Draft #172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Draft #172
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename this file (class_diagram_cpp.puml?), move it to docs/, remove Design/ directory.
| BorderColor<<mock>> IndianRed | ||
| } | ||
|
|
||
| class IKvsBuilder <<implementation>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> KvsBuilder - no longer an interface.
|
|
||
| class IKvsBuilder <<implementation>> { | ||
| - instance_id: InstanceId | ||
| - need_defaults : bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> -defaults : KvsDefaults
| class IKvsBuilder <<implementation>> { | ||
| - instance_id: InstanceId | ||
| - need_defaults : bool | ||
| - need_kvs : bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> -kvs_load : KvsLoad
| + snapshot_count: Result<size_t> | ||
| + snapshot_max_count() : Result<size_t> | ||
| + snapshot_restore(snapshot_id:SnapshotId): ResultBlank | ||
| + get_kvs_file_name(snapshot_id:SnapshotId): Result<filesystem::path> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove - filesystem related.
| + set_value(Key:string, value:KvsValue): ResultBlank | ||
| + remove_key(Key:string) : ResultBlank | ||
| + flush() : ResultBlank | ||
| + snapshot_count: Result<size_t> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a function.
| IStorageBackEnd -[hidden]right- IKvs | ||
|
|
||
| ' IStorageBackEnd interface (top middle-right) | ||
| interface IStorageBackEnd <<interface>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> IStorageBackend?
| ' Notes matching original design | ||
| note left of IKvsBuilder | ||
| builder will contain static map for opened instances | ||
| single tone should be applied to the builder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's a single tone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding names - please align with https://github.com/qorix-group/persistency/blob/e3945d4bea1bb3627dd46835db403b0c460306d1/docs/class_diagram.puml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this file is not included in any index file
| @startuml NewDesign | ||
|
|
||
| ' Define color schemes matching original design | ||
| skinparam class { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why overriding the global design? this will result in an inconsistent documentation.
| + Load_kvs(instance_id:InstanceId, snapshot_id:SnapshotId):IKvs | ||
| + Load_default(instance_id:InstanceId) | ||
| + Flush(instance_id:InstanceId, kvs:unordered_map<string,kvs> | ||
| + Snapshot_count(instance_id:InstanceId):Result<size_t> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function name shall follow snake_case convention
| @@ -0,0 +1,158 @@ | |||
| @startuml NewDesign | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove or rename NewDesign to something more meanigful
| + parser : unique_ptr<IjsonParser> parser | ||
| + writer : unique_ptr<IjsonWriter> writer | ||
| + logger : unique_ptr<mw::log::logger> logger | ||
| + Storage_backend : IStorageBackEnd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relation (Composition) to IStorageBackEnd is not modelled in this class diagramm
| + default_kvs : unordered_map<string, KvsValue> | ||
| + filename_prefix : score::filesystem::path | ||
| + filesystem : score::filesystem | ||
| + parser : unique_ptr<IjsonParser> parser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall the parser and reader not be abstracted by the Backend?
| + snapshot_count: Result<size_t> | ||
| + snapshot_max_count() : Result<size_t> | ||
| + snapshot_restore(snapshot_id:SnapshotId): ResultBlank | ||
| + get_kvs_file_name(snapshot_id:SnapshotId): Result<filesystem::path> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need them on public interface? If yes, naming is inconsistent file_name vs filename
No description provided.