feat(config): add FactConfigBuilder for better semantics#135
feat(config): add FactConfigBuilder for better semantics#135
Conversation
04c7e8a to
a306241
Compare
173d401 to
8743383
Compare
a306241 to
e219451
Compare
8743383 to
3e499d2
Compare
The new added FactConfigBuilder makes it obvious that the way of building a configuration is by parsing a set of files and putting together a FactConfig object from them and the CLI arguments. This improves semantics and makes the code more streamlined.
3e499d2 to
e7e3c00
Compare
| self | ||
| } | ||
|
|
||
| pub(super) fn files(&self) -> &[PathBuf] { |
There was a problem hiding this comment.
This is a bit of a nit, so feel free to tell me where to go with this one, but this feels like it's breaking the builder pattern a fair bit. The builder should be a bit more ephemeral (and it might even make sense for build to consume the builder)
But these files (as PathBuf) are used extensively later on, long after the builder has been used to build the config.
Perhaps we could expose this from the config itself since it is the one that owns the content of these files and should know which files it got that information from? WDYT?
There was a problem hiding this comment.
but this feels like it's breaking the builder pattern a fair bit. The builder should be a bit more ephemeral (and it might even make sense for
buildto consume the builder)
This makes sense when you want a builder that is a one-off that gets destroyed when you are done using it, but in this case we would want to call .build() every time we need to reload our configuration, in that case we don't want to have to construct the entire builder again. Sure, the builder now is super simple, it only takes the static list of files, but it may grow later on to.
Perhaps we could expose this from the config itself since it is the one that owns the content of these files and should know which files it got that information from? WDYT?
This is similar to what the current implementation does, the list of files is static right now, so it is just a list held privately in the config module. Both the reloader and the the config objects can just use the list as needed this way.
For the time being, I think the existing implementation on main is good enough, maybe we can leave this PR alone for a bit and see where the configuration of fact goes in the future, maybe we'll take this builder approach back up again, maybe we'll just close the PR in a few months.
Description
The new added FactConfigBuilder makes it obvious that the way of building a configuration is by parsing a set of files and putting together a FactConfig object from them and the CLI arguments. This improves semantics and makes the code more streamlined.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Current integration tests should cover the changes.