Repository navigation
Conversation
Maleware
moved this from Development: In Progress
to Development: Waiting for Review
in Stackable Engineering
Techassi
moved this from Development: Waiting for Review
to Development: In Review
in Stackable Engineering
Techassi
left a comment
Techassi
left a comment
There was a problem hiding this comment.
A bunch of comments.
I would also argue that the same thing can be achieved with TryFrom, making this whole trait obsolete.
| /// Turns a role group's merged config into the validated config the build step works with. | ||
| trait IntoValidated { |
There was a problem hiding this comment.
I would argue that this trait is not tied to role(s) and/or role group(s) at all. This trait can be applied to any "turn this into a validated that".
|
|
||
| /// Turns a role group's merged config into the validated config the build step works with. | ||
| trait IntoValidated { | ||
| /// `ValidatedConfig` of the `Config` to validate. |
There was a problem hiding this comment.
The docs mention ValidatedConfig, but this type is called Validated. Also, the doc comment should again be more generic, like "The validated type".
| type ConfigError; | ||
|
|
||
| /// Consumes the merged config, as nothing needs it once it is validated. | ||
| fn validate(self) -> Result<Self::Validated, Self::ConfigError>; |
There was a problem hiding this comment.
The trait is called IntoValidated and as such, this function should also be named into_validated
| fn validate(self) -> Result<Self::Validated, Self::ConfigError>; | |
| fn into_validated(self) -> Result<Self::Validated, Self::ConfigError>; |
There was a problem hiding this comment.
Yeah, but we usually do something like
validated = RawStruct.validate();I like how this reads and like to keep the function name. Better if we rename to trait Validation ?
There was a problem hiding this comment.
If we keep this trait, and prefer validate over into_validated, the trait should be named Validate.
| use crate::role_utils::{Role, RoleGroup}; | ||
|
|
||
| /// Turns a role group's merged config into the validated config the build step works with. | ||
| trait IntoValidated { |
There was a problem hiding this comment.
This trait is not public and thus cannot be used outside of this crate, which I guess is not the intention.
Co-authored-by: Techassi <git@techassi.dev>
Co-authored-by: Techassi <git@techassi.dev>
…/operator-rs into feat/IntoValidated
Description
Adds trait
IntoValidatedused in hdfs-operator for further usage across operators.Definition of Done Checklist
Author
Reviewer
Acceptance