-
Notifications
You must be signed in to change notification settings - Fork 13.4k
modularize the config module bootstrap #141272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
modularize the config module bootstrap #141272
Conversation
This PR modifies If appropriate, please update |
This comment has been minimized.
This comment has been minimized.
6a1e868
to
b8374d6
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the general direction of this. Could you perhaps split the changes into multiple commits though? It would be easier to review. Thank you :)
… with the logic for deserializing and merging them
…level methods that interact across different config sections
b8374d6
to
ad21211
Compare
Kind of crazy just how much code is there in config definition and parsing. The changes LGTM, but since this is a large-ish change, I'd like to hear also from other members of t-bootstrap if they are OK with it. CC @onur-ozkan @jieyouxu. |
/// Since we use `#[serde(deny_unknown_fields)]` on `TomlConfig`, we need a wrapper type | ||
/// for the "change-id" field to parse it even if other fields are invalid. This ensures | ||
/// that if deserialization fails due to other fields, we can still provide the changelogs | ||
/// to allow developers to potentially find the reason for the failure in the logs.. | ||
#[derive(Deserialize, Default)] | ||
pub(crate) struct ChangeIdWrapper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have types
module but this module also includes bunch of types. Right now it seems like there is no clear distinction between these modules and people will start putting things all around without having a clear clue. If we want to have module for per task, we have to make it super explicit/clear and have it well documented.
// NOTE: can't derive(Deserialize) because the intermediate trip through toml::Value only | ||
// deserializes i64, and derive() only generates visit_u64 | ||
impl<'de> Deserialize<'de> for DebuginfoLevel { | ||
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> | ||
where | ||
D: Deserializer<'de>, | ||
{ | ||
use serde::de::Error; | ||
|
||
Ok(match Deserialize::deserialize(deserializer)? { | ||
StringOrInt::String(s) if s == "none" => DebuginfoLevel::None, | ||
StringOrInt::Int(0) => DebuginfoLevel::None, | ||
StringOrInt::String(s) if s == "line-directives-only" => { | ||
DebuginfoLevel::LineDirectivesOnly | ||
} | ||
StringOrInt::String(s) if s == "line-tables-only" => DebuginfoLevel::LineTablesOnly, | ||
StringOrInt::String(s) if s == "limited" => DebuginfoLevel::Limited, | ||
StringOrInt::Int(1) => DebuginfoLevel::Limited, | ||
StringOrInt::String(s) if s == "full" => DebuginfoLevel::Full, | ||
StringOrInt::Int(2) => DebuginfoLevel::Full, | ||
StringOrInt::Int(n) => { | ||
let other = serde::de::Unexpected::Signed(n); | ||
return Err(D::Error::invalid_value(other, &"expected 0, 1, or 2")); | ||
} | ||
StringOrInt::String(s) => { | ||
let other = serde::de::Unexpected::Str(&s); | ||
return Err(D::Error::invalid_value( | ||
other, | ||
&"expected none, line-tables-only, limited, or full", | ||
)); | ||
} | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a parsing logic.
@rustbot author |
Reminder, once the PR becomes ready for a review, use |
cc @clubby789 |
@rustbot review |
Vibes-wise, organization seems fine to me. |
I still have this concern (also see this as an additional ref). With the current approach, adding new things will always be unclear for some people. Even now, for example, we have What I would suggest instead is organizing the modules based on the configuration field. For example, With this structure it would make it very explicit where things belong and anything that doesn't fit this rule can be easily caught during PR reviews. |
Right now, our
config
module is pretty big, sitting at over 3,000 lines and doing quite a lot. This PR aims to break it down into smaller, more focused modules to make it easier to understand and work with:types
: This will hold all the core data structures for our configuration.toml
: This module will contain structures that mirror ourbootstrap.toml
file, along with the logic for deserializing and merging them.config
(main file): This will declare the centralConfig
struct and house its top-level methods that interact across different config sections.parsing
: This is where the main logic for loading and validating the configuration will live.r? @Kobzol