Skip to content

Commit baf91fa

Browse files
committed
refactor: simplify Diagnostic to enum, extract for_each_dep_table_mut
Replace flat Diagnostic struct (6 Option fields + CheckKind) with DiagnosticKind enum carrying variant-specific data. Add mutable variant of for_each_dep_table to eliminate duplicated iteration in fix_member_dep.
1 parent 38426c1 commit baf91fa

5 files changed

Lines changed: 204 additions & 166 deletions

File tree

src/check.rs

Lines changed: 53 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use std::collections::{BTreeMap, HashMap};
22

3-
use crate::diagnostic::{CheckKind, Diagnostic, Severity};
3+
use crate::diagnostic::{Diagnostic, DiagnosticKind, Severity};
44
use crate::workspace::WorkspaceInfo;
55

66
pub fn run_checks(workspace: &WorkspaceInfo, promotion_threshold: usize) -> Vec<Diagnostic> {
@@ -23,21 +23,29 @@ pub fn run_checks(workspace: &WorkspaceInfo, promotion_threshold: usize) -> Vec<
2323
let lookup_name = dep.package.as_deref().unwrap_or(&dep.name);
2424

2525
if let Some(ws_dep) = workspace.workspace_deps.get(lookup_name) {
26-
let check = match (&dep.version, &ws_dep.version) {
27-
(Some(dep_ver), Some(ws_ver)) if dep_ver == ws_ver => CheckKind::NotInherited,
28-
(Some(_), Some(_)) => CheckKind::VersionMismatch,
29-
_ => CheckKind::NotInherited,
26+
let kind = match (&dep.version, &ws_dep.version) {
27+
(Some(dep_ver), Some(ws_ver)) if dep_ver == ws_ver => {
28+
DiagnosticKind::NotInherited {
29+
version: dep.version.clone(),
30+
member: member_rel.clone(),
31+
workspace_version: ws_dep.version.clone(),
32+
}
33+
}
34+
(Some(_), Some(_)) => DiagnosticKind::VersionMismatch {
35+
version: dep.version.clone(),
36+
member: member_rel.clone(),
37+
workspace_version: ws_dep.version.clone(),
38+
},
39+
_ => DiagnosticKind::NotInherited {
40+
version: dep.version.clone(),
41+
member: member_rel.clone(),
42+
workspace_version: ws_dep.version.clone(),
43+
},
3044
};
3145
diagnostics.push(Diagnostic {
3246
severity: Severity::Error,
33-
check,
3447
dependency: lookup_name.to_string(),
35-
version: dep.version.clone(),
36-
member: Some(member_rel.clone()),
37-
workspace_version: ws_dep.version.clone(),
38-
count: None,
39-
members: None,
40-
suggested_version: None,
48+
kind,
4149
});
4250
} else {
4351
dep_usage
@@ -63,14 +71,12 @@ pub fn run_checks(workspace: &WorkspaceInfo, promotion_threshold: usize) -> Vec<
6371

6472
diagnostics.push(Diagnostic {
6573
severity: Severity::Warning,
66-
check: CheckKind::PromotionCandidate,
6774
dependency: dep_name.clone(),
68-
version: None,
69-
member: None,
70-
workspace_version: None,
71-
count: Some(usages.len()),
72-
members: Some(usages.iter().map(|(m, _)| m.clone()).collect()),
73-
suggested_version,
75+
kind: DiagnosticKind::PromotionCandidate {
76+
count: usages.len(),
77+
members: usages.iter().map(|(m, _)| m.clone()).collect(),
78+
suggested_version,
79+
},
7480
});
7581
}
7682
}
@@ -97,7 +103,10 @@ mod tests {
97103
let diags = run_checks(&ws, 2);
98104
assert_eq!(diags.len(), 1);
99105
assert_eq!(diags[0].dependency, "serde");
100-
assert!(matches!(diags[0].check, CheckKind::NotInherited));
106+
assert!(matches!(
107+
diags[0].kind,
108+
DiagnosticKind::NotInherited { .. }
109+
));
101110
}
102111

103112
#[test]
@@ -106,9 +115,19 @@ mod tests {
106115
let diags = run_checks(&ws, 2);
107116
assert_eq!(diags.len(), 1);
108117
assert_eq!(diags[0].dependency, "rand");
109-
assert!(matches!(diags[0].check, CheckKind::VersionMismatch));
110-
assert_eq!(diags[0].version.as_deref(), Some("0.7"));
111-
assert_eq!(diags[0].workspace_version.as_deref(), Some("0.8"));
118+
assert!(matches!(
119+
diags[0].kind,
120+
DiagnosticKind::VersionMismatch { .. }
121+
));
122+
if let DiagnosticKind::VersionMismatch {
123+
version,
124+
workspace_version,
125+
..
126+
} = &diags[0].kind
127+
{
128+
assert_eq!(version.as_deref(), Some("0.7"));
129+
assert_eq!(workspace_version.as_deref(), Some("0.8"));
130+
}
112131
}
113132

114133
#[test]
@@ -117,8 +136,13 @@ mod tests {
117136
let diags = run_checks(&ws, 2);
118137
assert_eq!(diags.len(), 1);
119138
assert_eq!(diags[0].dependency, "serde_yaml");
120-
assert!(matches!(diags[0].check, CheckKind::PromotionCandidate));
121-
assert_eq!(diags[0].count, Some(2));
139+
assert!(matches!(
140+
diags[0].kind,
141+
DiagnosticKind::PromotionCandidate { .. }
142+
));
143+
if let DiagnosticKind::PromotionCandidate { count, .. } = &diags[0].kind {
144+
assert_eq!(*count, 2);
145+
}
122146
}
123147

124148
#[test]
@@ -147,6 +171,9 @@ mod tests {
147171
let diags = run_checks(&ws, 2);
148172
assert_eq!(diags.len(), 1);
149173
assert_eq!(diags[0].dependency, "winapi");
150-
assert!(matches!(diags[0].check, CheckKind::NotInherited));
174+
assert!(matches!(
175+
diags[0].kind,
176+
DiagnosticKind::NotInherited { .. }
177+
));
151178
}
152179
}

src/diagnostic.rs

Lines changed: 73 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,5 @@
11
use serde::Serialize;
22

3-
#[derive(Debug, Clone, Serialize)]
4-
#[serde(rename_all = "kebab-case")]
5-
pub enum CheckKind {
6-
NotInherited,
7-
VersionMismatch,
8-
PromotionCandidate,
9-
}
10-
113
#[derive(Debug, Clone, PartialEq, Eq, Serialize)]
124
#[serde(rename_all = "lowercase")]
135
pub enum Severity {
@@ -18,44 +10,63 @@ pub enum Severity {
1810
#[derive(Debug, Clone, Serialize)]
1911
pub struct Diagnostic {
2012
pub severity: Severity,
21-
pub check: CheckKind,
2213
pub dependency: String,
23-
#[serde(skip_serializing_if = "Option::is_none")]
24-
pub version: Option<String>,
25-
#[serde(skip_serializing_if = "Option::is_none")]
26-
pub member: Option<String>,
27-
#[serde(skip_serializing_if = "Option::is_none")]
28-
pub workspace_version: Option<String>,
29-
#[serde(skip_serializing_if = "Option::is_none")]
30-
pub count: Option<usize>,
31-
#[serde(skip_serializing_if = "Option::is_none")]
32-
pub members: Option<Vec<String>>,
33-
#[serde(skip_serializing_if = "Option::is_none")]
34-
pub suggested_version: Option<String>,
14+
#[serde(flatten)]
15+
pub kind: DiagnosticKind,
16+
}
17+
18+
#[derive(Debug, Clone, Serialize)]
19+
#[serde(tag = "check", rename_all = "kebab-case")]
20+
pub enum DiagnosticKind {
21+
NotInherited {
22+
#[serde(skip_serializing_if = "Option::is_none")]
23+
version: Option<String>,
24+
member: String,
25+
#[serde(skip_serializing_if = "Option::is_none")]
26+
workspace_version: Option<String>,
27+
},
28+
VersionMismatch {
29+
#[serde(skip_serializing_if = "Option::is_none")]
30+
version: Option<String>,
31+
member: String,
32+
#[serde(skip_serializing_if = "Option::is_none")]
33+
workspace_version: Option<String>,
34+
},
35+
PromotionCandidate {
36+
count: usize,
37+
members: Vec<String>,
38+
#[serde(skip_serializing_if = "Option::is_none")]
39+
suggested_version: Option<String>,
40+
},
3541
}
3642

3743
impl Diagnostic {
3844
pub fn format_human(&self) -> String {
39-
match self.check {
40-
CheckKind::NotInherited => {
41-
let ver = self.version.as_deref().unwrap_or("?");
42-
let member = self.member.as_deref().unwrap_or("?");
45+
match &self.kind {
46+
DiagnosticKind::NotInherited { version, member, .. } => {
47+
let ver = version.as_deref().unwrap_or("?");
4348
format!(
4449
"error: `{dep} = \"{ver}\"` in {member} should use `{dep} = {{ workspace = true }}`",
4550
dep = self.dependency,
4651
)
4752
}
48-
CheckKind::VersionMismatch => {
49-
let ver = self.version.as_deref().unwrap_or("?");
50-
let member = self.member.as_deref().unwrap_or("?");
51-
let ws_ver = self.workspace_version.as_deref().unwrap_or("?");
53+
DiagnosticKind::VersionMismatch {
54+
version,
55+
member,
56+
workspace_version,
57+
} => {
58+
let ver = version.as_deref().unwrap_or("?");
59+
let ws_ver = workspace_version.as_deref().unwrap_or("?");
5260
format!(
5361
"error: `{dep} = \"{ver}\"` in {member} has a different version than workspace `{dep} = \"{ws_ver}\"`",
5462
dep = self.dependency,
5563
)
5664
}
57-
CheckKind::PromotionCandidate => {
58-
let count = self.count.unwrap_or(0);
65+
DiagnosticKind::PromotionCandidate {
66+
count,
67+
members,
68+
suggested_version,
69+
} => {
5970
let severity = match self.severity {
6071
Severity::Error => "error",
6172
Severity::Warning => "warning",
@@ -64,12 +75,10 @@ impl Diagnostic {
6475
"{severity}: `{}` appears in {count} crates but is not in [workspace.dependencies]",
6576
self.dependency,
6677
)];
67-
if let Some(members) = &self.members {
68-
for m in members {
69-
lines.push(format!(" --> {m}"));
70-
}
78+
for m in members {
79+
lines.push(format!(" --> {m}"));
7180
}
72-
if let Some(ver) = &self.suggested_version {
81+
if let Some(ver) = suggested_version {
7382
lines.push(format!(
7483
" hint: consider adding `{} = \"{}\"` to [workspace.dependencies]",
7584
self.dependency, ver,
@@ -140,14 +149,12 @@ mod tests {
140149
fn test_not_inherited_human_format() {
141150
let d = Diagnostic {
142151
severity: Severity::Error,
143-
check: CheckKind::NotInherited,
144152
dependency: "lru".into(),
145-
version: Some("0.12".into()),
146-
member: Some("crates/crypto/Cargo.toml".into()),
147-
workspace_version: Some("0.12".into()),
148-
count: None,
149-
members: None,
150-
suggested_version: None,
153+
kind: DiagnosticKind::NotInherited {
154+
version: Some("0.12".into()),
155+
member: "crates/crypto/Cargo.toml".into(),
156+
workspace_version: Some("0.12".into()),
157+
},
151158
};
152159
let output = d.format_human();
153160
assert!(output.contains("error:"));
@@ -160,14 +167,12 @@ mod tests {
160167
fn test_version_mismatch_human_format() {
161168
let d = Diagnostic {
162169
severity: Severity::Error,
163-
check: CheckKind::VersionMismatch,
164170
dependency: "rand".into(),
165-
version: Some("0.7".into()),
166-
member: Some("crates/utils/Cargo.toml".into()),
167-
workspace_version: Some("0.8".into()),
168-
count: None,
169-
members: None,
170-
suggested_version: None,
171+
kind: DiagnosticKind::VersionMismatch {
172+
version: Some("0.7".into()),
173+
member: "crates/utils/Cargo.toml".into(),
174+
workspace_version: Some("0.8".into()),
175+
},
171176
};
172177
let output = d.format_human();
173178
assert!(output.contains("error:"));
@@ -180,17 +185,15 @@ mod tests {
180185
fn test_promotion_candidate_human_format() {
181186
let d = Diagnostic {
182187
severity: Severity::Warning,
183-
check: CheckKind::PromotionCandidate,
184188
dependency: "serde_yaml".into(),
185-
version: None,
186-
member: None,
187-
workspace_version: None,
188-
count: Some(3),
189-
members: Some(vec![
190-
"crates/config/Cargo.toml".into(),
191-
"crates/node/Cargo.toml".into(),
192-
]),
193-
suggested_version: Some("0.9".into()),
189+
kind: DiagnosticKind::PromotionCandidate {
190+
count: 3,
191+
members: vec![
192+
"crates/config/Cargo.toml".into(),
193+
"crates/node/Cargo.toml".into(),
194+
],
195+
suggested_version: Some("0.9".into()),
196+
},
194197
};
195198
let output = d.format_human();
196199
assert!(output.contains("warning:"));
@@ -203,14 +206,12 @@ mod tests {
203206
fn test_report_summary_human() {
204207
let report = DiagnosticReport::new(vec![Diagnostic {
205208
severity: Severity::Error,
206-
check: CheckKind::NotInherited,
207209
dependency: "lru".into(),
208-
version: Some("0.12".into()),
209-
member: Some("crates/crypto/Cargo.toml".into()),
210-
workspace_version: Some("0.12".into()),
211-
count: None,
212-
members: None,
213-
suggested_version: None,
210+
kind: DiagnosticKind::NotInherited {
211+
version: Some("0.12".into()),
212+
member: "crates/crypto/Cargo.toml".into(),
213+
workspace_version: Some("0.12".into()),
214+
},
214215
}]);
215216
let output = report.format_human();
216217
assert!(output.contains("1 error"));
@@ -220,14 +221,12 @@ mod tests {
220221
fn test_report_json() {
221222
let report = DiagnosticReport::new(vec![Diagnostic {
222223
severity: Severity::Error,
223-
check: CheckKind::NotInherited,
224224
dependency: "lru".into(),
225-
version: Some("0.12".into()),
226-
member: Some("crates/crypto/Cargo.toml".into()),
227-
workspace_version: Some("0.12".into()),
228-
count: None,
229-
members: None,
230-
suggested_version: None,
225+
kind: DiagnosticKind::NotInherited {
226+
version: Some("0.12".into()),
227+
member: "crates/crypto/Cargo.toml".into(),
228+
workspace_version: Some("0.12".into()),
229+
},
231230
}]);
232231
let json = report.format_json();
233232
let parsed: serde_json::Value = serde_json::from_str(&json).unwrap();

0 commit comments

Comments
 (0)