Repository navigation
Conversation
rustbot
added
A-configuration
|
r? @weihanglo rustbot has assigned @weihanglo. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
| Path, | ||
| /// A value defined in an environment variable. | ||
| #[expect(dead_code, reason = "file loading does not use environment values")] | ||
| Environment, |
There was a problem hiding this comment.
Do we have a plan of setting these two new DefintionKind elsewhere? Without a plan I am not sure what that would be.
There was a problem hiding this comment.
I added these solely because I re-framed this as a <enum>Kind which I felt made the API more cohesive / gave these a better name.
I am wondering about making Definition an opaque type with
fn kind(&self) -> DefinitionKindthough maybe that would be better namedfn precedence(&self) -> Precedence- a
Sourceenum which would beFile(Path) | Env(String) | None
There was a problem hiding this comment.
Making it opaque sounds fine to me.
a
Sourceenum which would beFile(Path) | Env(String) | None
Better! But still not extremely satisfied. Like our Source for dependencies is the actual source of the dependency, where this is like early mentioned a sticky-bit enum tracking the first loaded source. Our precedent of this is FeaturesFor (which is a bad name unfortunately).
If we cannot come up with a better one, I am good with Source.
In working to move `ConfigStore` out, I realized I had left behind the `config.rs` name instead of naming it `config_values.rs`. In preparing that, I realized I should have used a more distinct name rather than being one letter off from `ConfigValue`. My main aim for the name is to convey that this is not the actual config (that is deserialized) but the raw data.
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This branch has not been deployed
3 participants
What does this PR try to resolve?
This is part of #12883
By pulling out the raw ConfigStore API into its own module, we get a clearer picture of cross-module boundaries for what will eventually be cross-package boundaries.
The next step is to pull out deserialization. Once that is done, there is enough value to pull this all out into a separate package.
Notes to reviewers
🤖 LLM involvement: for trivial refactor steps