Skip to content

Conversation

@Molter73
Copy link
Collaborator

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

  • Investigated and inspected CI test results
  • Updated documentation accordingly

Automated testing

  • Added unit tests
  • Added integration tests
  • Added regression tests

If any of these don't apply, please comment below.

Testing Performed

Current integration tests should cover the changes.

@Molter73 Molter73 force-pushed the mauro/ROX-30836/hotreload-paths branch from 04c7e8a to a306241 Compare October 30, 2025 09:55
@Molter73 Molter73 force-pushed the mauro/config-builder branch from 173d401 to 8743383 Compare October 30, 2025 10:24
@Molter73 Molter73 force-pushed the mauro/ROX-30836/hotreload-paths branch from a306241 to e219451 Compare October 30, 2025 10:46
@Molter73 Molter73 force-pushed the mauro/config-builder branch from 8743383 to 3e499d2 Compare October 30, 2025 10:50
Base automatically changed from mauro/ROX-30836/hotreload-paths to main October 30, 2025 11:05
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.
@Molter73 Molter73 force-pushed the mauro/config-builder branch from 3e499d2 to e7e3c00 Compare October 30, 2025 11:43
self
}

pub(super) fn files(&self) -> &[PathBuf] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants