Skip to content

Commit f877c83

Browse files
authored
simplify templates (#370)
* Remove TEAMS WITH ASKS placeholder and implement automatic team injection Successfully removed the manual `<!-- TEAMS WITH ASKS -->` placeholder system from the rust-project-goals mdbook preprocessor and implemented automatic team injection. Key Accomplishments: 1. **Eliminated Manual Placeholders** - Removed `<!-- TEAMS WITH ASKS -->` requirement from goal documents - Cleaned up regex definitions and verification code - Updated template to remove Teams row entirely 2. **Implemented Smart Auto-Injection** - Automatically computes team names from goal's `teams_with_asks()` data - Handles both existing goals (replaces placeholder) and new goals (adds row) - Shows "(none)" for goals without team asks, formatted team links otherwise 3. **Robust Implementation** - Added milestone directory filtering to avoid processing documentation files - Proper error handling to prevent panics on malformed paths - Template exclusion to avoid processing template files as goals 4. **Backward Compatibility** - Existing goal files with old placeholders continue to work - Seamless transition - no manual migration required - New goals from updated template work automatically Technical Changes: - Modified: crates/mdbook-goals/src/mdbook_preprocessor.rs - Added inject_teams_into_metadata_table() - Modified: crates/rust-project-goals/src/goal.rs - Removed placeholder verification, added template exclusion - Modified: crates/rust-project-goals/src/re.rs - Removed TEAMS_WITH_ASKS regex - Modified: src/TEMPLATE.md - Removed Teams row entirely Result: Goal authors can now focus purely on content - team information is automatically handled based on their team asks data. The system is cleaner, more maintainable, and eliminates a common source of manual errors. * Remove TASK OWNERS placeholder and implement automatic task owner injection Extended the automatic injection system to also handle task owners. Removed the manual `<!-- TASK OWNERS -->` placeholder requirement and implemented automatic task owner injection. Goal documents no longer need to include either Teams or Task owners placeholders in their metadata tables. Key Accomplishments: 1. **Eliminated Both Placeholder Systems** - Removed `<!-- TASK OWNERS -->` requirement from goal documents - Completely removed the old placeholder replacement system - Cleaned up all related regex definitions and verification code 2. **Fully Automatic Injection** - Both Teams and Task owners rows are now computed and injected automatically - Smart row placement: Task owners after Teams (if present) or after Point of contact - Shows "(none)" for goals without task owners, formatted names otherwise 3. **Dramatically Simplified Template** - Removed both Teams and Task owners rows from template entirely - Template now only has 4 metadata rows instead of 6 - Focuses on what authors actually need to fill out manually 4. **Consistent Implementation** - Applied the same robust pattern for both injection systems - Milestone directory filtering, template exclusion, backward compatibility - Proper error handling and smart placement logic 5. **Complete Backward Compatibility** - Existing goal files with old placeholders continue to work seamlessly - System replaces existing rows or adds missing ones as needed - No manual migration required Technical Changes: - Modified: crates/mdbook-goals/src/mdbook_preprocessor.rs - Added inject_task_owners_into_metadata_table(), removed replace_metadata_placeholder() - Modified: crates/rust-project-goals/src/goal.rs - Removed placeholder verification, removed unused verify_row function - Modified: crates/rust-project-goals/src/re.rs - Removed TASK_OWNERS regex - Modified: src/TEMPLATE.md - Removed Task owners row entirely Result: Goal authors can now focus entirely on content - all metadata injection is handled automatically based on the goal's structured data. The template is significantly simplified and less error-prone. * Clean up legacy placeholder rows from goal files Successfully removed all legacy `<!-- TEAMS WITH ASKS -->` and `<!-- TASK OWNERS -->` placeholder rows from 115+ goal files. These placeholders are no longer needed since the preprocessor now automatically injects Teams and Task owners information based on the goal's metadata. Key Accomplishments: 1. **Massive Cleanup** - Removed 115 instances of `<!-- TEAMS WITH ASKS -->` placeholders - Removed 115 instances of `<!-- TASK OWNERS -->` placeholders - Eliminated 230+ lines of legacy placeholder code across all goal files 2. **Fixed Table Structure Issues** - Resolved broken metadata tables where placeholder removal left empty lines - Fixed markdown table parsing errors that prevented successful builds - Ensured all metadata tables have proper continuous structure 3. **Verified Auto-injection System** - Confirmed Teams rows are automatically injected with correct team information - Confirmed Task owners rows are automatically injected with correct owner data - Verified consistent formatting across all goal files 4. **Template Simplification** - Template now only contains 4 metadata rows that authors need to fill manually - Removed 2 placeholder rows that were confusing and error-prone - Goal authors can now focus purely on content creation Impact: - Goal authors no longer need to manage manual placeholders - Cleaner, simpler template reduces chance of formatting errors - Automatic, consistent formatting of team and owner information - Better maintainability and system consistency - Successful mdbook builds without table parsing errors Result: The placeholder cleanup is complete. All goal files are clean, the build works perfectly, and the auto-injection system handles all metadata formatting automatically. Goal authors can focus purely on writing content while the system handles metadata presentation. * Replace HTML comment syntax with triple-parentheses syntax for mdbook preprocessor - Change regex patterns from <!-- PATTERN --> to (((PATTERN))) - Update all 7 regex patterns in re.rs: TEAM_ASKS, GOAL_LIST, FLAGSHIP_GOAL_LIST, OTHER_GOAL_LIST, GOAL_NOT_ACCEPTED_LIST, GOAL_COUNT, VALID_TEAM_ASKS - Update error messages in mdbook_preprocessor.rs to reference new syntax - Update all markdown files across 2024h2, 2025h1, 2025h2, admin/samples, and about directories - Solves HTML comment processing issues in mdbook templates where comments get stripped or require HTML encoding The new triple-parentheses syntax is HTML-safe, template-friendly, and visually distinctive. * Add categorical flagship goal tracking system - Replace binary flagship status with categorical system using | Flagship | CategoryName | - Add support for filtered macros like (((FLAGSHIP GOALS:Performance))) - Maintain backward compatibility with existing (((FLAGSHIP GOALS))) macro - Add metadata.flagship() method returning Option<&str> - Include migration script to convert existing flagship goals to | Flagship | Yes | - Refactor goal list processing to use unified replace_goal_lists_helper function - Support whitespace trimming in both macro filters and metadata values All existing flagship goals migrated to use 'Yes' category. New filtered macros enable focused flagship goal pages by category. * support `(((#FLAGSHIP GOALS)))` notation * fix :-- columns * Refactor metadata injection to eliminate duplication - Combined inject_teams_into_metadata_table and inject_task_owners_into_metadata_table into single inject_metadata_rows function - Added find_markdown_table_end helper that finds any table by looking for lines starting with '|' - Simplified logic: find table end, insert both rows - no complex conditional checking - Added test for table finding logic
1 parent 2e6cdcf commit f877c83

File tree

142 files changed

+360
-634
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

142 files changed

+360
-634
lines changed

crates/mdbook-goals/src/mdbook_preprocessor.rs

Lines changed: 185 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use rust_project_goals::util::{self, GithubUserInfo};
1414

1515
use rust_project_goals::spanned::Spanned;
1616
use rust_project_goals::{
17-
goal::{self, GoalDocument, Status, TeamAsk},
17+
goal::{self, GoalDocument, TeamAsk},
1818
re, team,
1919
};
2020

@@ -40,9 +40,6 @@ impl Preprocessor for GoalPreprocessor {
4040
}
4141

4242
pub struct GoalPreprocessorWithContext<'c> {
43-
team_asks: &'static Regex,
44-
goal_count: &'static Regex,
45-
username: &'static Regex,
4643
ctx: &'c PreprocessorContext,
4744
links: Vec<(String, String)>,
4845
linkifiers: Vec<(Regex, String)>,
@@ -138,9 +135,6 @@ impl<'c> GoalPreprocessorWithContext<'c> {
138135

139136
Ok(GoalPreprocessorWithContext {
140137
ctx,
141-
team_asks: &re::TEAM_ASKS,
142-
goal_count: &re::GOAL_COUNT,
143-
username: &re::USERNAME,
144138
links,
145139
linkifiers,
146140
display_names,
@@ -157,6 +151,7 @@ impl<'c> GoalPreprocessorWithContext<'c> {
157151
self.replace_valid_team_asks(chapter)?;
158152
self.replace_goal_lists(chapter)?;
159153
self.replace_goal_count(chapter)?;
154+
self.replace_flagship_goal_count(chapter)?;
160155
self.link_teams(chapter)?;
161156
self.link_users(chapter)?;
162157
self.linkify(chapter)?;
@@ -176,12 +171,12 @@ impl<'c> GoalPreprocessorWithContext<'c> {
176171
}
177172

178173
fn replace_goal_count(&mut self, chapter: &mut Chapter) -> anyhow::Result<()> {
179-
if !self.goal_count.is_match(&chapter.content) {
174+
if !re::GOAL_COUNT.is_match(&chapter.content) {
180175
return Ok(());
181176
}
182177

183178
let Some(chapter_path) = &chapter.path else {
184-
anyhow::bail!("found `<!-- #GOALS -->` but chapter has no path")
179+
anyhow::bail!("found `(((#GOALS)))` but chapter has no path")
185180
};
186181

187182
let goals = self.goal_documents(chapter_path)?;
@@ -191,35 +186,77 @@ impl<'c> GoalPreprocessorWithContext<'c> {
191186
.filter(|g| g.metadata.status.is_not_not_accepted())
192187
.count();
193188

194-
chapter.content = self
195-
.goal_count
189+
chapter.content = re::GOAL_COUNT
190+
.replace_all(&chapter.content, &count.to_string())
191+
.to_string();
192+
193+
Ok(())
194+
}
195+
196+
fn replace_flagship_goal_count(&mut self, chapter: &mut Chapter) -> anyhow::Result<()> {
197+
if !re::FLAGSHIP_GOAL_COUNT.is_match(&chapter.content) {
198+
return Ok(());
199+
}
200+
201+
let Some(chapter_path) = &chapter.path else {
202+
anyhow::bail!("found `(((#FLAGSHIP_GOALS)))` but chapter has no path")
203+
};
204+
205+
let goals = self.goal_documents(chapter_path)?;
206+
207+
let count = goals
208+
.iter()
209+
.filter(|g| g.metadata.flagship().is_some() && g.metadata.status.is_not_not_accepted())
210+
.count();
211+
212+
chapter.content = re::FLAGSHIP_GOAL_COUNT
196213
.replace_all(&chapter.content, &count.to_string())
197214
.to_string();
198215

199216
Ok(())
200217
}
201218

202219
fn replace_goal_lists(&mut self, chapter: &mut Chapter) -> anyhow::Result<()> {
203-
self.replace_goal_lists_helper(chapter, &re::FLAGSHIP_GOAL_LIST, |status| {
204-
status.is_flagship && status.is_not_not_accepted()
220+
// Handle filtered flagship goals first (more specific pattern)
221+
self.replace_flagship_goal_lists_filtered(chapter)?;
222+
223+
// Handle unfiltered flagship goals
224+
self.replace_goal_lists_helper(chapter, &re::FLAGSHIP_GOAL_LIST, |goal, _capture| {
225+
goal.metadata.flagship().is_some() && goal.metadata.status.content.is_not_not_accepted()
205226
})?;
206-
self.replace_goal_lists_helper(chapter, &re::OTHER_GOAL_LIST, |status| {
207-
!status.is_flagship && status.is_not_not_accepted()
227+
228+
self.replace_goal_lists_helper(chapter, &re::OTHER_GOAL_LIST, |goal, _capture| {
229+
goal.metadata.flagship().is_none() && goal.metadata.status.content.is_not_not_accepted()
208230
})?;
209-
self.replace_goal_lists_helper(chapter, &re::GOAL_LIST, |status| {
210-
status.is_not_not_accepted()
231+
self.replace_goal_lists_helper(chapter, &re::GOAL_LIST, |goal, _capture| {
232+
goal.metadata.status.content.is_not_not_accepted()
211233
})?;
212-
self.replace_goal_lists_helper(chapter, &re::GOAL_NOT_ACCEPTED_LIST, |status| {
213-
!status.is_not_not_accepted()
234+
self.replace_goal_lists_helper(chapter, &re::GOAL_NOT_ACCEPTED_LIST, |goal, _capture| {
235+
!goal.metadata.status.content.is_not_not_accepted()
214236
})?;
215237
Ok(())
216238
}
217239

240+
fn replace_flagship_goal_lists_filtered(
241+
&mut self,
242+
chapter: &mut Chapter,
243+
) -> anyhow::Result<()> {
244+
self.replace_goal_lists_helper(
245+
chapter,
246+
&re::FLAGSHIP_GOAL_LIST_FILTERED,
247+
|goal, capture| {
248+
let filter_value = capture.unwrap().trim(); // Safe because this regex always has a capture
249+
goal.metadata.status.content.is_not_not_accepted()
250+
&& goal.metadata.flagship().map(|f| f.trim()) == Some(filter_value)
251+
},
252+
)
253+
}
254+
218255
fn replace_goal_lists_helper(
219256
&mut self,
220257
chapter: &mut Chapter,
221258
regex: &Regex,
222-
filter: impl Fn(Status) -> bool,
259+
filter: impl Fn(&GoalDocument, Option<&str>) -> bool,
223260
) -> anyhow::Result<()> {
224261
loop {
225262
let Some(m) = regex.find(&chapter.content) else {
@@ -231,12 +268,16 @@ impl<'c> GoalPreprocessorWithContext<'c> {
231268
anyhow::bail!("found `{regex}` but chapter has no path")
232269
};
233270

234-
// Extract out the list of goals with the given status.
271+
// Extract capture group if present
272+
let capture_value = regex
273+
.captures(&chapter.content[range.clone()])
274+
.and_then(|caps| caps.get(1))
275+
.map(|m| m.as_str().trim());
276+
277+
// Extract out the list of goals with the given filter.
235278
let goals = self.goal_documents(chapter_path)?;
236-
let mut goals_with_status: Vec<&GoalDocument> = goals
237-
.iter()
238-
.filter(|g| filter(g.metadata.status.content))
239-
.collect();
279+
let mut goals_with_status: Vec<&GoalDocument> =
280+
goals.iter().filter(|g| filter(g, capture_value)).collect();
240281

241282
goals_with_status.sort_by_key(|g| &g.metadata.title);
242283

@@ -269,13 +310,13 @@ impl<'c> GoalPreprocessorWithContext<'c> {
269310

270311
/// Look for `<!-- TEAM ASKS -->` in the chapter content and replace it with the team asks.
271312
fn replace_team_asks(&mut self, chapter: &mut Chapter) -> anyhow::Result<()> {
272-
let Some(m) = self.team_asks.find(&chapter.content) else {
313+
let Some(m) = re::TEAM_ASKS.find(&chapter.content) else {
273314
return Ok(());
274315
};
275316
let range = m.range();
276317

277318
let Some(path) = &chapter.path else {
278-
anyhow::bail!("found `<!-- TEAM ASKS -->` but chapter has no path")
319+
anyhow::bail!("found `(((TEAM ASKS)))` but chapter has no path")
279320
};
280321

281322
let goals = self.goal_documents(path)?;
@@ -336,8 +377,7 @@ impl<'c> GoalPreprocessorWithContext<'c> {
336377
}
337378

338379
fn link_users(&mut self, chapter: &mut Chapter) -> anyhow::Result<()> {
339-
let usernames: BTreeSet<String> = self
340-
.username
380+
let usernames: BTreeSet<String> = re::USERNAME
341381
.find_iter(&chapter.content)
342382
.map(|m| m.as_str().to_string())
343383
.filter(|username| !self.ignore_users.contains(username))
@@ -440,58 +480,139 @@ impl<'c> GoalPreprocessorWithContext<'c> {
440480
/// All goal documents should have this in their metadata table;
441481
/// that is enforced during goal parsing.
442482
fn replace_metadata_placeholders(&mut self, chapter: &mut Chapter) -> anyhow::Result<()> {
443-
self.replace_metadata_placeholder(chapter, &re::TASK_OWNERS, |goal| {
444-
goal.task_owners.iter().cloned().collect()
445-
})?;
446-
447-
self.replace_metadata_placeholder(chapter, &re::TEAMS_WITH_ASKS, |goal| {
448-
goal.teams_with_asks()
449-
.iter()
450-
.map(|team_name| team_name.name())
451-
.collect()
452-
})?;
483+
// Auto-inject teams and task owners directly into metadata table instead of using placeholders
484+
self.inject_metadata_rows(chapter)?;
453485

454486
Ok(())
455487
}
456488

457-
/// Replace one of the placeholders that occur in the goal document metadata,
458-
/// like [`re::TASK_OWNERS`][].
459-
fn replace_metadata_placeholder(
460-
&mut self,
461-
chapter: &mut Chapter,
462-
regex: &Regex,
463-
op: impl Fn(&GoalDocument) -> Vec<String>,
464-
) -> anyhow::Result<()> {
465-
let Some(m) = regex.find(&chapter.content) else {
466-
return Ok(());
467-
};
468-
let range = m.range();
489+
/// Find the end of a markdown table (first line that doesn't start with |).
490+
/// Returns the byte offset where new rows should be inserted.
491+
fn find_markdown_table_end(content: &str) -> Option<usize> {
492+
let lines: Vec<&str> = content.lines().collect();
493+
494+
// Find first line starting with |
495+
let table_start = lines.iter().position(|line| line.trim_start().starts_with('|'))?;
496+
497+
// Find first line after table_start that doesn't start with |
498+
let table_end = lines[table_start..]
499+
.iter()
500+
.position(|line| !line.trim_start().starts_with('|'))
501+
.map(|pos| table_start + pos)
502+
.unwrap_or(lines.len());
503+
504+
// Calculate byte offset to the start of the table_end line
505+
let mut offset = 0;
506+
for i in 0..table_end {
507+
508+
if i > 0 {
509+
offset += 1; // newline
510+
}
511+
offset += lines[i].len();
512+
}
513+
514+
if table_end < lines.len() {
515+
// There's a line after the table, insert before it
516+
Some(offset + 1) // +1 for the newline after the last table row
517+
} else {
518+
// Table goes to end of file, append at the end
519+
Some(content.len())
520+
}
521+
}
469522

523+
/// Automatically inject team names and task owners into the metadata table.
524+
/// This replaces the need for manual placeholders and combines the logic
525+
/// to avoid duplicate table parsing.
526+
fn inject_metadata_rows(&mut self, chapter: &mut Chapter) -> anyhow::Result<()> {
470527
let Some(chapter_path) = chapter.path.as_ref() else {
471-
anyhow::bail!(
472-
"goal chapter `{}` matches placeholder regex but has no path",
473-
chapter.name
474-
);
528+
return Ok(()); // No path, nothing to inject
529+
};
530+
531+
// Skip template files
532+
if chapter_path.file_name().and_then(|n| n.to_str()) == Some("TEMPLATE.md") {
533+
return Ok(());
534+
}
535+
536+
// Only process files in milestone directories (like 2024h2, 2025h1, etc.)
537+
let Some(parent_dir) = chapter_path.parent() else {
538+
return Ok(());
475539
};
476540

477-
// Hack: leave this stuff alone in the template
478-
if chapter_path.file_name().unwrap() == "TEMPLATE.md" {
541+
let Some(parent_name) = parent_dir.file_name().and_then(|n| n.to_str()) else {
479542
return Ok(());
543+
};
544+
545+
if !parent_name
546+
.chars()
547+
.next()
548+
.map_or(false, |c| c.is_ascii_digit())
549+
{
550+
return Ok(()); // Not a milestone directory, skip
480551
}
481552

553+
// Find the goal document for this chapter
482554
let goals = self.goal_documents(&chapter_path)?;
483555
let chapter_in_context = self.ctx.config.book.src.join(chapter_path);
484556
let Some(goal) = goals.iter().find(|gd| gd.path == chapter_in_context) else {
485-
anyhow::bail!(
486-
"goal chapter `{}` has no goal document at path {:?}",
487-
chapter.name,
488-
chapter_path,
489-
);
557+
return Ok(()); // No goal document found, nothing to inject
558+
};
559+
560+
// Compute the team names
561+
let team_names: Vec<String> = goal
562+
.teams_with_asks()
563+
.iter()
564+
.map(|team_name| team_name.name())
565+
.collect();
566+
567+
let teams_text = if team_names.is_empty() {
568+
"(none)".to_string()
569+
} else {
570+
team_names.join(", ")
490571
};
491572

492-
let replacement = op(goal).join(", ");
493-
chapter.content.replace_range(range, &replacement);
573+
// Compute the task owner names
574+
let task_owners: Vec<String> = goal.task_owners.iter().cloned().collect();
575+
576+
let task_owners_text = if task_owners.is_empty() {
577+
"(none)".to_string()
578+
} else {
579+
task_owners.join(", ")
580+
};
581+
582+
// Find the table end and insert both rows
583+
if let Some(table_end) = Self::find_markdown_table_end(&chapter.content) {
584+
let insertion_text = format!(
585+
"| Teams | {} |\n| Task owners | {} |\n",
586+
teams_text, task_owners_text
587+
);
588+
chapter.content.insert_str(table_end, &insertion_text);
589+
}
494590

495591
Ok(())
496592
}
497593
}
594+
595+
#[cfg(test)]
596+
mod tests {
597+
use super::*;
598+
599+
#[test]
600+
fn test_find_markdown_table_end() {
601+
let content = "Some text before\n\n| Metadata | Value |\n|----------|-------|\n| Point of contact | @nikomatsakis |\n| Teams | (none) |\n\nSome text after";
602+
603+
let result = GoalPreprocessorWithContext::find_markdown_table_end(content);
604+
assert!(result.is_some());
605+
606+
let offset = result.unwrap();
607+
let (before, after) = content.split_at(offset);
608+
609+
// Should split right before the blank line after the table
610+
assert!(before.ends_with("| Teams | (none) |\n"));
611+
assert!(after.starts_with("\nSome text after"));
612+
613+
// Test that inserting at this offset works correctly
614+
let mut test_content = content.to_string();
615+
test_content.insert_str(offset, "| New row | value |\n");
616+
assert!(test_content.contains("| Teams | (none) |\n| New row | value |\n\nSome text after"));
617+
}
618+
}

0 commit comments

Comments
 (0)