Make background validation performed by Services configurable #2060 #2150
Conversation
brungardtdb
force-pushed
the
brungardtdb/configure_background_validation
branch
from
4f8be06
to
288fb26
Compare
|
Thank you for this pull request, @brungardtdb! I'm currently on vacation (for the rest of the year). I might take a look at this next week, when I get a chance, or at the latest in January, when I'm back to regular work. |
…. Then passed the validation config into the validation service.
…d to the validation service, and added validate_with_config() method to object.rs
brungardtdb
force-pushed
the
brungardtdb/configure_background_validation
branch
from
288fb26
to
c411a63
Compare
There was a problem hiding this comment.
Thank you again for the pull request, @brungardtdb! And I'm very sorry that it took me so long to review it. There was a confluence of factors that caused this. Usually, things around here work much more quickly!
I've left a few comments. Most are just me nit-picking, and fine to leave as-is (as noted in the respective comment). The only thing I'd like to see changed before merging, is the DerefMut implementation.
crates/fj-core/src/services/mod.rs
Outdated
| @@ -42,6 +42,18 @@ impl Services { | |||
| } | |||
| } | |||
|
|
|||
| /// Construct an instance of `Services` with a pre-defined configuration for the validation service | |||
| pub fn with_validation_config(config: &ValidationConfig) -> Self { | |||
There was a problem hiding this comment.
Seems a bit misleading to take a &ValidationConfig that is then copied inside. It would be better to directly take ValidationConfig.
But this is not a blocker. Happy to merge as-is!
There was a problem hiding this comment.
This has been addressed.
| /// Optional validation config | ||
| config: Option<ValidationConfig>, |
There was a problem hiding this comment.
It's probably simpler to instead have config: ValidationConfig, and initialize that with its default value, if not provided.
But this would be a nice-to-have. Perfectly fine to merge this as-is.
There was a problem hiding this comment.
This has also been addressed.
| impl<S: State> DerefMut for Service<S> { | ||
| fn deref_mut(&mut self) -> &mut Self::Target { | ||
| &mut self.state | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this is used by the new Services constructor, which in itself seems fine. But this provides direct access to service state to all code, which seems error-prone.
I think there are two better ways to do this:
- Define a command for updating the validation config (see the existing
ValidationCommand), make the execution of that command emitValidationEvent::ConfigurationDefined, and useService::executeto update the validation. - In the new
Servicesconstructor, instead of usingService::<Validation>::default(), initialize the validation service withService::new, which can be provided with aValidationthat has the configuration set properly.
Solution 1 is more verbose, and probably more flexible than we need. So I'd prefer solution 2. What do you think?
There was a problem hiding this comment.
I think I agree with you, solution 1 seems like it might be overkill, so I will try my hand at solution 2 and see how that goes. 👍
There was a problem hiding this comment.
@hannobraun , I believe I have managed to implement all of the changes that you had suggested. Please take a look and let me know if this is what you were going for! 😄
No worries! I think I submitted this PR over the holidays, so I wasn't expecting to hear back right away. 😄 |
Passed in ValidationConfig directly instead of cloning a reference. Removed optional ValidationConfig for ValidationService, use default or config passed in through alternate constructor. Deleted DerefMut implementation.
|
@hannobraun, thank you! |
Added functionality to pass in a predefined validation configuration into Services so that it can be used in the Validation Service as outlined in #2060.