From 126792f940344be92d3b95b241bd685db9808579 Mon Sep 17 00:00:00 2001 From: l Date: Mon, 1 Dec 2025 12:31:32 +0000 Subject: [PATCH 01/28] count changed files --- src/bin/pr-metadata-validator.rs | 18 ++++++++++++++++++ src/prs.rs | 7 +++++++ 2 files changed, 25 insertions(+) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index f0cc5f0..8f5d1e3 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -84,6 +84,7 @@ async fn main() { &format!("{}{}", BAD_TITLE_COMMENT_PREFIX, reason) } ValidationResult::UnknownRegion => UNKNOWN_REGION_COMMENT, + ValidationResult::WrongFileCount => WRONG_FILE_COUNT, }; let full_message = format!( @@ -138,12 +139,17 @@ const UNKNOWN_REGION_COMMENT: &str = r#"Your PR's title didn't contain a known r Please check the expected title format, and make sure your region is in the correct place and spelled correctly."#; +const WRONG_FILE_COUNT: &str = r#"The changed files in this PR don't match what is expected for this task. + +Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints."#; + enum ValidationResult { Ok, BodyTemplateNotFilledOut, CouldNotMatch, BadTitleFormat { reason: String }, UnknownRegion, + WrongFileCount } async fn validate_pr( @@ -234,6 +240,18 @@ async fn validate_pr( return Ok(ValidationResult::BodyTemplateNotFilledOut); } + + // get unique descriptor of the task solved in this PR + let example_max_changes_count = 1; + println!("{}", pr_in_question.changed_files); + if pr_in_question.changed_files > example_max_changes_count { + return Ok(ValidationResult::WrongFileCount); + } + // get list of expected changed files for this task + // if this pr has specific expected changes, + // get list of changed files for this pr + // make sure they match + Ok(ValidationResult::Ok) } diff --git a/src/prs.rs b/src/prs.rs index f726925..f53319c 100644 --- a/src/prs.rs +++ b/src/prs.rs @@ -24,6 +24,7 @@ pub struct Pr { pub updated_at: DateTime, pub is_closed: bool, pub labels: BTreeSet, + pub changed_files: u64, } #[derive(Clone, Debug, PartialEq, Eq, Serialize)] @@ -93,6 +94,7 @@ pub async fn get_prs( title, state, body, + changed_files, .. }| { // If a user is deleted from GitHub, their User will be None - ignore PRs from deleted users. @@ -119,6 +121,10 @@ pub async fn get_prs( let url = html_url?.to_string(); let title = title?; let body = body.unwrap_or_default(); + let changed_files = match changed_files { + Some(changes) => changes, + None => 99999 + }; Some(Pr { number, @@ -131,6 +137,7 @@ pub async fn get_prs( body, is_closed, labels, + changed_files }) }, ) From e3da750b31c18cf3d56b6cb52b2c5f3250bc90e8 Mon Sep 17 00:00:00 2001 From: l Date: Mon, 8 Dec 2025 11:20:42 +0000 Subject: [PATCH 02/28] basic file match check --- src/bin/pr-metadata-validator.rs | 29 ++++++++++++++++------------- src/prs.rs | 11 ++--------- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index 8f5d1e3..732857c 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -84,7 +84,7 @@ async fn main() { &format!("{}{}", BAD_TITLE_COMMENT_PREFIX, reason) } ValidationResult::UnknownRegion => UNKNOWN_REGION_COMMENT, - ValidationResult::WrongFileCount => WRONG_FILE_COUNT, + ValidationResult::WrongFiles => WRONG_FILES, }; let full_message = format!( @@ -139,7 +139,7 @@ const UNKNOWN_REGION_COMMENT: &str = r#"Your PR's title didn't contain a known r Please check the expected title format, and make sure your region is in the correct place and spelled correctly."#; -const WRONG_FILE_COUNT: &str = r#"The changed files in this PR don't match what is expected for this task. +const WRONG_FILES: &str = r#"The changed files in this PR don't match what is expected for this task. Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints."#; @@ -149,7 +149,7 @@ enum ValidationResult { CouldNotMatch, BadTitleFormat { reason: String }, UnknownRegion, - WrongFileCount + WrongFiles } async fn validate_pr( @@ -240,17 +240,20 @@ async fn validate_pr( return Ok(ValidationResult::BodyTemplateNotFilledOut); } - - // get unique descriptor of the task solved in this PR - let example_max_changes_count = 1; - println!("{}", pr_in_question.changed_files); - if pr_in_question.changed_files > example_max_changes_count { - return Ok(ValidationResult::WrongFileCount); + // get all of the metadata here + let mut pr_files = match octocrab + .pulls(github_org_name, module_name) + .list_files(pr_number) + .await { + Ok(p) => p.into_iter(), + Err(_) => return Ok(ValidationResult::WrongFiles) // todo probably needs a separate error condition. + }; + let re_for_pr = Regex::new(r"^Sprint-1/1-key-exercises").unwrap(); + while let Some(pr_file) = pr_files.next() { + if !re_for_pr.is_match(&pr_file.filename) { + return Ok(ValidationResult::WrongFiles) + } } - // get list of expected changed files for this task - // if this pr has specific expected changes, - // get list of changed files for this pr - // make sure they match Ok(ValidationResult::Ok) } diff --git a/src/prs.rs b/src/prs.rs index f53319c..cb8609b 100644 --- a/src/prs.rs +++ b/src/prs.rs @@ -23,8 +23,7 @@ pub struct Pr { pub state: PrState, pub updated_at: DateTime, pub is_closed: bool, - pub labels: BTreeSet, - pub changed_files: u64, + pub labels: BTreeSet } #[derive(Clone, Debug, PartialEq, Eq, Serialize)] @@ -94,7 +93,6 @@ pub async fn get_prs( title, state, body, - changed_files, .. }| { // If a user is deleted from GitHub, their User will be None - ignore PRs from deleted users. @@ -121,10 +119,6 @@ pub async fn get_prs( let url = html_url?.to_string(); let title = title?; let body = body.unwrap_or_default(); - let changed_files = match changed_files { - Some(changes) => changes, - None => 99999 - }; Some(Pr { number, @@ -136,8 +130,7 @@ pub async fn get_prs( title, body, is_closed, - labels, - changed_files + labels }) }, ) From 8c8bdae531da31aebcd1c4c6a269595af36c497e Mon Sep 17 00:00:00 2001 From: l Date: Mon, 8 Dec 2025 13:44:11 +0000 Subject: [PATCH 03/28] get metadata from an issue --- src/bin/pr-metadata-validator.rs | 53 ++++++++++++++++++++++++++------ 1 file changed, 44 insertions(+), 9 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index 732857c..a63f58c 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -240,22 +240,57 @@ async fn validate_pr( return Ok(ValidationResult::BodyTemplateNotFilledOut); } - // get all of the metadata here + match check_pr_file_changes(octocrab, github_org_name, module_name, pr_number) + .await { + Some(result) => return Ok(result), + None => () + } + + Ok(ValidationResult::Ok) +} + +// Check the changed files in a pull request match what is expected for that sprint task +async fn check_pr_file_changes( + octocrab: &Octocrab, + github_org_name: &str, + module_name: &str, + pr_number: u64, +) -> Option { + // Get the Sprint Task's description of expected changes + let task_issue = match octocrab + .issues(github_org_name, module_name) + .get(35) // TODO get the correct one,just default to this for testing now + .await { + Ok(iss) => iss, + Err(_) => return Some(ValidationResult::CouldNotMatch) // Failed to find the right task + }; + let task_issue_body = match task_issue.body { + Some(body) => body, + None => return None // Task is empty, nothing left to check + }; + let directory_description = Regex::new("CHANGE_DIR=(.+)\\n").unwrap(); + let directory_description_regex = match directory_description.captures(&task_issue_body) { + Some(capts) => capts.get(0).unwrap().as_str(), // Only allows a single directory for now + None => return None // There is no match defined for this task, don't do any more checks + }; + let directory_matcher = match Regex::new(directory_description_regex) { + Ok(regex) => regex, + Err(_) => return Some(ValidationResult::WrongFiles) // The regex is not valid. Return an error so the curriculum owner can address this + }; + // Get all of the changed files and check them let mut pr_files = match octocrab .pulls(github_org_name, module_name) - .list_files(pr_number) + .list_files(pr_number) // TODO make sure this works across pages (+20 changed files? are there any tasks that meet this condition? is that an auto-fail?) .await { - Ok(p) => p.into_iter(), - Err(_) => return Ok(ValidationResult::WrongFiles) // todo probably needs a separate error condition. + Ok(page) => page.into_iter(), + Err(_) => return Some(ValidationResult::WrongFiles) // TODO probably needs a separate error condition. }; - let re_for_pr = Regex::new(r"^Sprint-1/1-key-exercises").unwrap(); while let Some(pr_file) = pr_files.next() { - if !re_for_pr.is_match(&pr_file.filename) { - return Ok(ValidationResult::WrongFiles) + if !directory_matcher.is_match(&pr_file.filename) { + return Some(ValidationResult::WrongFiles) } } - - Ok(ValidationResult::Ok) + return None; } struct KnownRegions(BTreeMap<&'static str, Vec<&'static str>>); From c331b6fbea53efbee83a925c449dd39a57204323 Mon Sep 17 00:00:00 2001 From: l Date: Tue, 9 Dec 2025 16:37:43 +0000 Subject: [PATCH 04/28] expand all pages of files --- src/bin/pr-metadata-validator.rs | 66 +++++++++++++++++++------------- 1 file changed, 40 insertions(+), 26 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index a63f58c..e3dccb3 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -1,5 +1,6 @@ use std::{collections::BTreeMap, process::exit}; +use anyhow::{anyhow, Context}; use chrono::NaiveDate; use indexmap::IndexMap; use maplit::btreemap; @@ -172,7 +173,7 @@ async fn validate_pr( .iter() .find(|pr| pr.number == pr_number) .ok_or_else(|| { - anyhow::anyhow!( + anyhow!( "Failed to find PR {} in list of PRs for module {}", pr_number, module_name @@ -240,10 +241,11 @@ async fn validate_pr( return Ok(ValidationResult::BodyTemplateNotFilledOut); } - match check_pr_file_changes(octocrab, github_org_name, module_name, pr_number) + match check_pr_file_changes(octocrab, github_org_name, module_name, pr_number, 35) // TODO get the correct one,just default to this for testing now .await { - Some(result) => return Ok(result), - None => () + Ok(Some(problem)) => return Ok(problem), + Ok(None) => (), + Err(e) => { let _ = anyhow!(e); } } Ok(ValidationResult::Ok) @@ -252,45 +254,57 @@ async fn validate_pr( // Check the changed files in a pull request match what is expected for that sprint task async fn check_pr_file_changes( octocrab: &Octocrab, - github_org_name: &str, + org_name: &str, module_name: &str, pr_number: u64, -) -> Option { + task_issue_number: u64, +) -> Result, Error> { // Get the Sprint Task's description of expected changes let task_issue = match octocrab - .issues(github_org_name, module_name) - .get(35) // TODO get the correct one,just default to this for testing now + .issues(org_name, module_name) + .get(task_issue_number) .await { Ok(iss) => iss, - Err(_) => return Some(ValidationResult::CouldNotMatch) // Failed to find the right task + Err(_) => return Ok(Some(ValidationResult::CouldNotMatch)) // Failed to find the right task }; let task_issue_body = match task_issue.body { Some(body) => body, - None => return None // Task is empty, nothing left to check + None => return Ok(None) // Task is empty, nothing left to check }; let directory_description = Regex::new("CHANGE_DIR=(.+)\\n").unwrap(); let directory_description_regex = match directory_description.captures(&task_issue_body) { - Some(capts) => capts.get(0).unwrap().as_str(), // Only allows a single directory for now - None => return None // There is no match defined for this task, don't do any more checks - }; - let directory_matcher = match Regex::new(directory_description_regex) { - Ok(regex) => regex, - Err(_) => return Some(ValidationResult::WrongFiles) // The regex is not valid. Return an error so the curriculum owner can address this + Some(capts) => capts.get(1).unwrap().as_str(), // Only allows a single directory for now + None => return Ok(None) // There is no match defined for this task, don't do any more checks }; - // Get all of the changed files and check them - let mut pr_files = match octocrab - .pulls(github_org_name, module_name) - .list_files(pr_number) // TODO make sure this works across pages (+20 changed files? are there any tasks that meet this condition? is that an auto-fail?) - .await { - Ok(page) => page.into_iter(), - Err(_) => return Some(ValidationResult::WrongFiles) // TODO probably needs a separate error condition. - }; + let directory_matcher = Regex::new(directory_description_regex) + .context("Invalid regex for task directory match")?; + // Get all of the changed files + let pr_files_pages = octocrab + .pulls(org_name, module_name) + .list_files(pr_number) + .await + .context("Failed to get changed files")?; + if pr_files_pages.items.len() == 0 { + return Ok(Some(ValidationResult::WrongFiles)); // no files committed + } + let pr_files_all = octocrab + .all_pages(pr_files_pages) + .await + .context("Failed to list all changed files")?; + let mut pr_files = pr_files_all + .into_iter(); + let mut i = 0; + // check each file and error if one is in unexpected place + println!("{}", directory_description_regex); while let Some(pr_file) = pr_files.next() { + i += 1; + println!("{}{}", i, pr_file.filename); if !directory_matcher.is_match(&pr_file.filename) { - return Some(ValidationResult::WrongFiles) + println!("Found bad match"); + return Ok(Some(ValidationResult::WrongFiles)) } } - return None; + return Ok(None); } struct KnownRegions(BTreeMap<&'static str, Vec<&'static str>>); From 41f026231f6e59d846a59b799d7d642ac91f898d Mon Sep 17 00:00:00 2001 From: l Date: Wed, 10 Dec 2025 13:52:04 +0000 Subject: [PATCH 05/28] add message and dynamically get issue --- src/bin/pr-metadata-validator.rs | 62 ++++++++++++++++++++++++++------ src/course.rs | 3 ++ 2 files changed, 54 insertions(+), 11 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index e3dccb3..f79db3a 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -85,7 +85,10 @@ async fn main() { &format!("{}{}", BAD_TITLE_COMMENT_PREFIX, reason) } ValidationResult::UnknownRegion => UNKNOWN_REGION_COMMENT, - ValidationResult::WrongFiles => WRONG_FILES, + ValidationResult::WrongFiles { files } => { + &format!("{}{}`", WRONG_FILES, files) + }, + ValidationResult::NoFiles => NO_FILES, }; let full_message = format!( @@ -142,7 +145,13 @@ Please check the expected title format, and make sure your region is in the corr const WRONG_FILES: &str = r#"The changed files in this PR don't match what is expected for this task. -Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints."#; +Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints. + +Please review the changed files tab at the top of the page, we are only expecting changes in this directory: `"#; + +const NO_FILES: &str = r#"This PR is missing any submitted files. + +Please check that you committed the right files and pushed to the repository"#; enum ValidationResult { Ok, @@ -150,7 +159,8 @@ enum ValidationResult { CouldNotMatch, BadTitleFormat { reason: String }, UnknownRegion, - WrongFiles + WrongFiles { files: String }, + NoFiles } async fn validate_pr( @@ -241,7 +251,40 @@ async fn validate_pr( return Ok(ValidationResult::BodyTemplateNotFilledOut); } - match check_pr_file_changes(octocrab, github_org_name, module_name, pr_number, 35) // TODO get the correct one,just default to this for testing now + + // TODO simplify and move into course or prs + let mut pr_assignment_descriptor = matched.sprints + .iter() + .map(|sprint_with_subs| sprint_with_subs.submissions.clone()) + .flatten() + .filter_map(|expected_or_some| match expected_or_some { + trainee_tracker::course::SubmissionState::Some(s) => Some(s), + _ => None + }) + .filter_map(|sub| match sub { + trainee_tracker::course::Submission::PullRequest { pull_request, assignment_descriptor, .. } => { + if pull_request.number == pr_number { + match assignment_descriptor { + trainee_tracker::course::Assignment::ExpectedPullRequest { html_url, ..} => Some(html_url), + _ => None + } + } else { + None + } + }, + _ => None + }); + + let issue_id_matcher = Regex::new("/(\\d+)$").unwrap(); + let mut pr_assignment_descriptor_id = 0; // TODO handle error if somehow this never gets set + while let Some(x) = pr_assignment_descriptor.next() { + pr_assignment_descriptor_id = match issue_id_matcher.captures(x.path()) { + Some(capts) => capts.get(1).unwrap().as_str().parse::().unwrap(), + None => { return Ok(ValidationResult::CouldNotMatch) } + }; + } + + match check_pr_file_changes(octocrab, github_org_name, module_name, pr_number, pr_assignment_descriptor_id) .await { Ok(Some(problem)) => return Ok(problem), Ok(None) => (), @@ -285,7 +328,7 @@ async fn check_pr_file_changes( .await .context("Failed to get changed files")?; if pr_files_pages.items.len() == 0 { - return Ok(Some(ValidationResult::WrongFiles)); // no files committed + return Ok(Some(ValidationResult::NoFiles)); // no files committed } let pr_files_all = octocrab .all_pages(pr_files_pages) @@ -293,15 +336,12 @@ async fn check_pr_file_changes( .context("Failed to list all changed files")?; let mut pr_files = pr_files_all .into_iter(); - let mut i = 0; // check each file and error if one is in unexpected place - println!("{}", directory_description_regex); while let Some(pr_file) = pr_files.next() { - i += 1; - println!("{}{}", i, pr_file.filename); if !directory_matcher.is_match(&pr_file.filename) { - println!("Found bad match"); - return Ok(Some(ValidationResult::WrongFiles)) + return Ok(Some(ValidationResult::WrongFiles { + files: directory_description_regex.to_string() + })) } } return Ok(None); diff --git a/src/course.rs b/src/course.rs index 93dd8aa..d931540 100644 --- a/src/course.rs +++ b/src/course.rs @@ -438,6 +438,7 @@ impl TraineeWithSubmissions { SubmissionState::Some(Submission::PullRequest { pull_request, optionality, + .. }) => { let max = match optionality { AssignmentOptionality::Mandatory => 10, @@ -539,6 +540,7 @@ pub enum Submission { PullRequest { pull_request: Pr, optionality: AssignmentOptionality, + assignment_descriptor: Assignment }, } @@ -1001,6 +1003,7 @@ fn match_pr_to_assignment( SubmissionState::Some(Submission::PullRequest { pull_request: pr, optionality, + assignment_descriptor: assignments[sprint_index].assignments[assignment_index].clone() }); } else if !pr.is_closed { unknown_prs.push(pr); From f9c16c60df2865a2b9362ce08da27dc404fb2139 Mon Sep 17 00:00:00 2001 From: l Date: Wed, 10 Dec 2025 15:33:41 +0000 Subject: [PATCH 06/28] move get descriptor logic to course --- src/bin/pr-metadata-validator.rs | 35 ++----------------------- src/course.rs | 44 +++++++++++++++++++++++++++++--- 2 files changed, 43 insertions(+), 36 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index f79db3a..6fb3dfd 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -9,7 +9,7 @@ use regex::Regex; use trainee_tracker::{ Error, config::{CourseSchedule, CourseScheduleWithRegisterSheetId}, - course::match_prs_to_assignments, + course::{match_prs_to_assignments, get_descriptor_id_for_pr}, newtypes::Region, octocrab::octocrab_for_token, prs::get_prs, @@ -251,38 +251,7 @@ async fn validate_pr( return Ok(ValidationResult::BodyTemplateNotFilledOut); } - - // TODO simplify and move into course or prs - let mut pr_assignment_descriptor = matched.sprints - .iter() - .map(|sprint_with_subs| sprint_with_subs.submissions.clone()) - .flatten() - .filter_map(|expected_or_some| match expected_or_some { - trainee_tracker::course::SubmissionState::Some(s) => Some(s), - _ => None - }) - .filter_map(|sub| match sub { - trainee_tracker::course::Submission::PullRequest { pull_request, assignment_descriptor, .. } => { - if pull_request.number == pr_number { - match assignment_descriptor { - trainee_tracker::course::Assignment::ExpectedPullRequest { html_url, ..} => Some(html_url), - _ => None - } - } else { - None - } - }, - _ => None - }); - - let issue_id_matcher = Regex::new("/(\\d+)$").unwrap(); - let mut pr_assignment_descriptor_id = 0; // TODO handle error if somehow this never gets set - while let Some(x) = pr_assignment_descriptor.next() { - pr_assignment_descriptor_id = match issue_id_matcher.captures(x.path()) { - Some(capts) => capts.get(1).unwrap().as_str().parse::().unwrap(), - None => { return Ok(ValidationResult::CouldNotMatch) } - }; - } + let pr_assignment_descriptor_id = get_descriptor_id_for_pr(matched.sprints, pr_number); match check_pr_file_changes(octocrab, github_org_name, module_name, pr_number, pr_assignment_descriptor_id) .await { diff --git a/src/course.rs b/src/course.rs index d931540..6c48f80 100644 --- a/src/course.rs +++ b/src/course.rs @@ -311,7 +311,7 @@ pub enum Assignment { }, ExpectedPullRequest { title: String, - html_url: Url, + html_url: Url, // TODO convert the task ID here optionality: AssignmentOptionality, }, } @@ -540,7 +540,7 @@ pub enum Submission { PullRequest { pull_request: Pr, optionality: AssignmentOptionality, - assignment_descriptor: Assignment + assignment_descriptor: u64 }, } @@ -992,6 +992,7 @@ fn match_pr_to_assignment( } } } + if let Some(Match { sprint_index, assignment_index, @@ -999,17 +1000,54 @@ fn match_pr_to_assignment( .. }) = best_match { + + let assignment_descriptor = assignments[sprint_index].assignments[assignment_index].clone(); + let pr_assignment_descriptor_id: u64 = match assignment_descriptor { + Assignment::ExpectedPullRequest { html_url, .. } => { + Regex::new("/(\\d+)$") + .unwrap() + .captures(html_url.path()) + .unwrap() + .get(1) + .unwrap() + .as_str() + .parse::() + .unwrap() + }, + _ => 0 + }; submissions[sprint_index].submissions[assignment_index] = SubmissionState::Some(Submission::PullRequest { pull_request: pr, optionality, - assignment_descriptor: assignments[sprint_index].assignments[assignment_index].clone() + assignment_descriptor: pr_assignment_descriptor_id }); } else if !pr.is_closed { unknown_prs.push(pr); } } +// Given a vector of sprints, and a target pr number, for a given person +// return the issue ID for the associated assignment descriptor +pub fn get_descriptor_id_for_pr(sprints: Vec, target_pr_number: u64) -> u64 { + match sprints + .iter() + .map(|sprint_with_subs| sprint_with_subs.submissions.clone()) + .flatten() + .filter_map(|missing_or_submission| match missing_or_submission { + SubmissionState::Some(s) => Some(s), + _ => None + }) + .find(|submission| match submission { + Submission::PullRequest { pull_request, .. } + => pull_request.number == target_pr_number, + _ => false + }) { + Some(Submission::PullRequest {assignment_descriptor, ..}) => assignment_descriptor, + _ => 0 // TODO once again, we need to handle this error better + } +} + fn make_title_more_matchable(title: &str) -> IndexSet { use itertools::Itertools; From 0a26bb65159540245c6bf96a89e672a290a6fdec Mon Sep 17 00:00:00 2001 From: l Date: Wed, 10 Dec 2025 15:41:18 +0000 Subject: [PATCH 07/28] Simplify get issue num logic --- src/course.rs | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/course.rs b/src/course.rs index 6c48f80..12e7c4f 100644 --- a/src/course.rs +++ b/src/course.rs @@ -141,6 +141,7 @@ fn parse_issue(issue: &Issue) -> Result labels, title, html_url, + number, .. } = issue; @@ -227,6 +228,7 @@ fn parse_issue(issue: &Issue) -> Result title: title.clone(), html_url: html_url.clone(), optionality, + assignment_descriptor_id: *number }), "Codility" => { // TODO: Handle these. @@ -311,7 +313,8 @@ pub enum Assignment { }, ExpectedPullRequest { title: String, - html_url: Url, // TODO convert the task ID here + html_url: Url, + assignment_descriptor_id: u64, optionality: AssignmentOptionality, }, } @@ -1001,19 +1004,8 @@ fn match_pr_to_assignment( }) = best_match { - let assignment_descriptor = assignments[sprint_index].assignments[assignment_index].clone(); - let pr_assignment_descriptor_id: u64 = match assignment_descriptor { - Assignment::ExpectedPullRequest { html_url, .. } => { - Regex::new("/(\\d+)$") - .unwrap() - .captures(html_url.path()) - .unwrap() - .get(1) - .unwrap() - .as_str() - .parse::() - .unwrap() - }, + let pr_assignment_descriptor_id: u64 = match assignments[sprint_index].assignments[assignment_index] { + Assignment::ExpectedPullRequest { assignment_descriptor_id, .. } => assignment_descriptor_id, _ => 0 }; submissions[sprint_index].submissions[assignment_index] = From 87c7330c6fd0bdbfe04dc26ef7567fa84b80ca24 Mon Sep 17 00:00:00 2001 From: l Date: Wed, 10 Dec 2025 15:42:34 +0000 Subject: [PATCH 08/28] cleanup comment --- src/course.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/course.rs b/src/course.rs index 12e7c4f..0801a3f 100644 --- a/src/course.rs +++ b/src/course.rs @@ -1036,7 +1036,7 @@ pub fn get_descriptor_id_for_pr(sprints: Vec, target_pr_n _ => false }) { Some(Submission::PullRequest {assignment_descriptor, ..}) => assignment_descriptor, - _ => 0 // TODO once again, we need to handle this error better + _ => 0 } } From 246a706ae7e5883495b2f973c4d4b05e7fcf9bb5 Mon Sep 17 00:00:00 2001 From: l Date: Wed, 10 Dec 2025 15:48:07 +0000 Subject: [PATCH 09/28] revert unchanged file --- src/prs.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/prs.rs b/src/prs.rs index cb8609b..f726925 100644 --- a/src/prs.rs +++ b/src/prs.rs @@ -23,7 +23,7 @@ pub struct Pr { pub state: PrState, pub updated_at: DateTime, pub is_closed: bool, - pub labels: BTreeSet + pub labels: BTreeSet, } #[derive(Clone, Debug, PartialEq, Eq, Serialize)] @@ -130,7 +130,7 @@ pub async fn get_prs( title, body, is_closed, - labels + labels, }) }, ) From 5fa207dac2b29767213b72cb0661aae963312681 Mon Sep 17 00:00:00 2001 From: l Date: Wed, 10 Dec 2025 15:51:37 +0000 Subject: [PATCH 10/28] resolve linter remarks --- src/bin/pr-metadata-validator.rs | 8 ++++---- src/course.rs | 3 +-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index 6fb3dfd..696d0b3 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -296,24 +296,24 @@ async fn check_pr_file_changes( .list_files(pr_number) .await .context("Failed to get changed files")?; - if pr_files_pages.items.len() == 0 { + if pr_files_pages.items.is_empty() { return Ok(Some(ValidationResult::NoFiles)); // no files committed } let pr_files_all = octocrab .all_pages(pr_files_pages) .await .context("Failed to list all changed files")?; - let mut pr_files = pr_files_all + let pr_files = pr_files_all .into_iter(); // check each file and error if one is in unexpected place - while let Some(pr_file) = pr_files.next() { + for pr_file in pr_files { if !directory_matcher.is_match(&pr_file.filename) { return Ok(Some(ValidationResult::WrongFiles { files: directory_description_regex.to_string() })) } } - return Ok(None); + Ok(None) } struct KnownRegions(BTreeMap<&'static str, Vec<&'static str>>); diff --git a/src/course.rs b/src/course.rs index 0801a3f..fcb8f12 100644 --- a/src/course.rs +++ b/src/course.rs @@ -1024,8 +1024,7 @@ fn match_pr_to_assignment( pub fn get_descriptor_id_for_pr(sprints: Vec, target_pr_number: u64) -> u64 { match sprints .iter() - .map(|sprint_with_subs| sprint_with_subs.submissions.clone()) - .flatten() + .flat_map(|sprint_with_subs| sprint_with_subs.submissions.clone()) .filter_map(|missing_or_submission| match missing_or_submission { SubmissionState::Some(s) => Some(s), _ => None From 40c29bb4d3df465ea648cfddae2350d5066ae8dc Mon Sep 17 00:00:00 2001 From: l Date: Wed, 10 Dec 2025 16:04:24 +0000 Subject: [PATCH 11/28] rustfmt --- src/bin/pr-metadata-validator.rs | 49 ++++++++++++++++++-------------- src/course.rs | 35 +++++++++++++---------- 2 files changed, 48 insertions(+), 36 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index 696d0b3..e468bba 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -1,6 +1,6 @@ use std::{collections::BTreeMap, process::exit}; -use anyhow::{anyhow, Context}; +use anyhow::{Context, anyhow}; use chrono::NaiveDate; use indexmap::IndexMap; use maplit::btreemap; @@ -9,7 +9,7 @@ use regex::Regex; use trainee_tracker::{ Error, config::{CourseSchedule, CourseScheduleWithRegisterSheetId}, - course::{match_prs_to_assignments, get_descriptor_id_for_pr}, + course::{get_descriptor_id_for_pr, match_prs_to_assignments}, newtypes::Region, octocrab::octocrab_for_token, prs::get_prs, @@ -85,9 +85,7 @@ async fn main() { &format!("{}{}", BAD_TITLE_COMMENT_PREFIX, reason) } ValidationResult::UnknownRegion => UNKNOWN_REGION_COMMENT, - ValidationResult::WrongFiles { files } => { - &format!("{}{}`", WRONG_FILES, files) - }, + ValidationResult::WrongFiles { files } => &format!("{}{}`", WRONG_FILES, files), ValidationResult::NoFiles => NO_FILES, }; @@ -160,7 +158,7 @@ enum ValidationResult { BadTitleFormat { reason: String }, UnknownRegion, WrongFiles { files: String }, - NoFiles + NoFiles, } async fn validate_pr( @@ -253,12 +251,21 @@ async fn validate_pr( let pr_assignment_descriptor_id = get_descriptor_id_for_pr(matched.sprints, pr_number); - match check_pr_file_changes(octocrab, github_org_name, module_name, pr_number, pr_assignment_descriptor_id) - .await { - Ok(Some(problem)) => return Ok(problem), - Ok(None) => (), - Err(e) => { let _ = anyhow!(e); } + match check_pr_file_changes( + octocrab, + github_org_name, + module_name, + pr_number, + pr_assignment_descriptor_id, + ) + .await + { + Ok(Some(problem)) => return Ok(problem), + Ok(None) => (), + Err(e) => { + let _ = anyhow!(e); } + } Ok(ValidationResult::Ok) } @@ -275,18 +282,19 @@ async fn check_pr_file_changes( let task_issue = match octocrab .issues(org_name, module_name) .get(task_issue_number) - .await { - Ok(iss) => iss, - Err(_) => return Ok(Some(ValidationResult::CouldNotMatch)) // Failed to find the right task - }; + .await + { + Ok(iss) => iss, + Err(_) => return Ok(Some(ValidationResult::CouldNotMatch)), // Failed to find the right task + }; let task_issue_body = match task_issue.body { Some(body) => body, - None => return Ok(None) // Task is empty, nothing left to check + None => return Ok(None), // Task is empty, nothing left to check }; let directory_description = Regex::new("CHANGE_DIR=(.+)\\n").unwrap(); let directory_description_regex = match directory_description.captures(&task_issue_body) { Some(capts) => capts.get(1).unwrap().as_str(), // Only allows a single directory for now - None => return Ok(None) // There is no match defined for this task, don't do any more checks + None => return Ok(None), // There is no match defined for this task, don't do any more checks }; let directory_matcher = Regex::new(directory_description_regex) .context("Invalid regex for task directory match")?; @@ -303,14 +311,13 @@ async fn check_pr_file_changes( .all_pages(pr_files_pages) .await .context("Failed to list all changed files")?; - let pr_files = pr_files_all - .into_iter(); + let pr_files = pr_files_all.into_iter(); // check each file and error if one is in unexpected place for pr_file in pr_files { if !directory_matcher.is_match(&pr_file.filename) { return Ok(Some(ValidationResult::WrongFiles { - files: directory_description_regex.to_string() - })) + files: directory_description_regex.to_string(), + })); } } Ok(None) diff --git a/src/course.rs b/src/course.rs index fcb8f12..16e8460 100644 --- a/src/course.rs +++ b/src/course.rs @@ -228,7 +228,7 @@ fn parse_issue(issue: &Issue) -> Result title: title.clone(), html_url: html_url.clone(), optionality, - assignment_descriptor_id: *number + assignment_descriptor_id: *number, }), "Codility" => { // TODO: Handle these. @@ -543,7 +543,7 @@ pub enum Submission { PullRequest { pull_request: Pr, optionality: AssignmentOptionality, - assignment_descriptor: u64 + assignment_descriptor: u64, }, } @@ -1003,16 +1003,19 @@ fn match_pr_to_assignment( .. }) = best_match { - - let pr_assignment_descriptor_id: u64 = match assignments[sprint_index].assignments[assignment_index] { - Assignment::ExpectedPullRequest { assignment_descriptor_id, .. } => assignment_descriptor_id, - _ => 0 - }; + let pr_assignment_descriptor_id: u64 = + match assignments[sprint_index].assignments[assignment_index] { + Assignment::ExpectedPullRequest { + assignment_descriptor_id, + .. + } => assignment_descriptor_id, + _ => 0, + }; submissions[sprint_index].submissions[assignment_index] = SubmissionState::Some(Submission::PullRequest { pull_request: pr, optionality, - assignment_descriptor: pr_assignment_descriptor_id + assignment_descriptor: pr_assignment_descriptor_id, }); } else if !pr.is_closed { unknown_prs.push(pr); @@ -1027,16 +1030,18 @@ pub fn get_descriptor_id_for_pr(sprints: Vec, target_pr_n .flat_map(|sprint_with_subs| sprint_with_subs.submissions.clone()) .filter_map(|missing_or_submission| match missing_or_submission { SubmissionState::Some(s) => Some(s), - _ => None + _ => None, }) .find(|submission| match submission { - Submission::PullRequest { pull_request, .. } - => pull_request.number == target_pr_number, - _ => false + Submission::PullRequest { pull_request, .. } => pull_request.number == target_pr_number, + _ => false, }) { - Some(Submission::PullRequest {assignment_descriptor, ..}) => assignment_descriptor, - _ => 0 - } + Some(Submission::PullRequest { + assignment_descriptor, + .. + }) => assignment_descriptor, + _ => 0, + } } fn make_title_more_matchable(title: &str) -> IndexSet { From 2a94e7188f6dccd26f6c3429a9079305d926c049 Mon Sep 17 00:00:00 2001 From: l Date: Wed, 17 Dec 2025 11:31:28 +0000 Subject: [PATCH 12/28] rename file var --- src/bin/pr-metadata-validator.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index e468bba..6d135a0 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -85,7 +85,7 @@ async fn main() { &format!("{}{}", BAD_TITLE_COMMENT_PREFIX, reason) } ValidationResult::UnknownRegion => UNKNOWN_REGION_COMMENT, - ValidationResult::WrongFiles { files } => &format!("{}{}`", WRONG_FILES, files), + ValidationResult::WrongFiles { expected_files_pattern } => &format!("{}`{}`", WRONG_FILES, expected_files_pattern), ValidationResult::NoFiles => NO_FILES, }; @@ -145,7 +145,7 @@ const WRONG_FILES: &str = r#"The changed files in this PR don't match what is ex Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints. -Please review the changed files tab at the top of the page, we are only expecting changes in this directory: `"#; +Please review the changed files tab at the top of the page, we are only expecting changes in this directory: "#; const NO_FILES: &str = r#"This PR is missing any submitted files. @@ -157,7 +157,7 @@ enum ValidationResult { CouldNotMatch, BadTitleFormat { reason: String }, UnknownRegion, - WrongFiles { files: String }, + WrongFiles { expected_files_pattern: String }, NoFiles, } @@ -316,7 +316,7 @@ async fn check_pr_file_changes( for pr_file in pr_files { if !directory_matcher.is_match(&pr_file.filename) { return Ok(Some(ValidationResult::WrongFiles { - files: directory_description_regex.to_string(), + expected_files_pattern: directory_description_regex.to_string(), })); } } From 727b8a122bc71af665fd0aabdbf0fb32399977e7 Mon Sep 17 00:00:00 2001 From: l Date: Wed, 17 Dec 2025 11:36:38 +0000 Subject: [PATCH 13/28] simplify reading issue body --- src/bin/pr-metadata-validator.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index 6d135a0..44d8b21 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -85,7 +85,9 @@ async fn main() { &format!("{}{}", BAD_TITLE_COMMENT_PREFIX, reason) } ValidationResult::UnknownRegion => UNKNOWN_REGION_COMMENT, - ValidationResult::WrongFiles { expected_files_pattern } => &format!("{}`{}`", WRONG_FILES, expected_files_pattern), + ValidationResult::WrongFiles { + expected_files_pattern, + } => &format!("{}`{}`", WRONG_FILES, expected_files_pattern), ValidationResult::NoFiles => NO_FILES, }; @@ -287,10 +289,7 @@ async fn check_pr_file_changes( Ok(iss) => iss, Err(_) => return Ok(Some(ValidationResult::CouldNotMatch)), // Failed to find the right task }; - let task_issue_body = match task_issue.body { - Some(body) => body, - None => return Ok(None), // Task is empty, nothing left to check - }; + let task_issue_body = task_issue.body.unwrap_or_default(); let directory_description = Regex::new("CHANGE_DIR=(.+)\\n").unwrap(); let directory_description_regex = match directory_description.captures(&task_issue_body) { Some(capts) => capts.get(1).unwrap().as_str(), // Only allows a single directory for now From 2285175d800c7c7c687af9e92f08573563175da4 Mon Sep 17 00:00:00 2001 From: l Date: Wed, 17 Dec 2025 11:48:29 +0000 Subject: [PATCH 14/28] reuse depaginator fn --- src/bin/pr-metadata-validator.rs | 21 +++++++++------------ src/octocrab.rs | 2 +- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index 44d8b21..0a5b4de 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -11,7 +11,7 @@ use trainee_tracker::{ config::{CourseSchedule, CourseScheduleWithRegisterSheetId}, course::{get_descriptor_id_for_pr, match_prs_to_assignments}, newtypes::Region, - octocrab::octocrab_for_token, + octocrab::{all_pages, octocrab_for_token}, prs::get_prs, }; @@ -298,19 +298,16 @@ async fn check_pr_file_changes( let directory_matcher = Regex::new(directory_description_regex) .context("Invalid regex for task directory match")?; // Get all of the changed files - let pr_files_pages = octocrab - .pulls(org_name, module_name) - .list_files(pr_number) - .await - .context("Failed to get changed files")?; - if pr_files_pages.items.is_empty() { + let pr_files = all_pages("changed files in pull request", octocrab, async || { + octocrab + .pulls(org_name, module_name) + .list_files(pr_number) + .await + }) + .await?; + if pr_files.is_empty() { return Ok(Some(ValidationResult::NoFiles)); // no files committed } - let pr_files_all = octocrab - .all_pages(pr_files_pages) - .await - .context("Failed to list all changed files")?; - let pr_files = pr_files_all.into_iter(); // check each file and error if one is in unexpected place for pr_file in pr_files { if !directory_matcher.is_match(&pr_file.filename) { diff --git a/src/octocrab.rs b/src/octocrab.rs index 480cb45..b05dc52 100644 --- a/src/octocrab.rs +++ b/src/octocrab.rs @@ -81,7 +81,7 @@ pub fn octocrab_for_token(token: String) -> Result { Ok(octocrab) } -pub(crate) async fn all_pages( +pub async fn all_pages( description: &str, octocrab: &Octocrab, func: impl AsyncFnOnce() -> Result, octocrab::Error>, From e97b701002cbd5d579761765933157a128e47a01 Mon Sep 17 00:00:00 2001 From: l Date: Wed, 17 Dec 2025 11:53:01 +0000 Subject: [PATCH 15/28] expect instead of unwrap --- src/bin/pr-metadata-validator.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index 0a5b4de..f1f719d 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -290,9 +290,13 @@ async fn check_pr_file_changes( Err(_) => return Ok(Some(ValidationResult::CouldNotMatch)), // Failed to find the right task }; let task_issue_body = task_issue.body.unwrap_or_default(); - let directory_description = Regex::new("CHANGE_DIR=(.+)\\n").unwrap(); + let directory_description = + Regex::new("CHANGE_DIR=(.+)\\n").expect("Known good regex failed to compile"); let directory_description_regex = match directory_description.captures(&task_issue_body) { - Some(capts) => capts.get(1).unwrap().as_str(), // Only allows a single directory for now + Some(capts) => capts + .get(1) + .expect("Regex capture failed to return string match") + .as_str(), // Only allows a single directory for now None => return Ok(None), // There is no match defined for this task, don't do any more checks }; let directory_matcher = Regex::new(directory_description_regex) From 5111e984e11856447d70461b627e4c502d8c4ec1 Mon Sep 17 00:00:00 2001 From: l Date: Wed, 17 Dec 2025 13:45:09 +0000 Subject: [PATCH 16/28] fix variable regex error handling --- src/bin/pr-metadata-validator.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index f1f719d..8015ab2 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -300,7 +300,8 @@ async fn check_pr_file_changes( None => return Ok(None), // There is no match defined for this task, don't do any more checks }; let directory_matcher = Regex::new(directory_description_regex) - .context("Invalid regex for task directory match")?; + .with_context(|| format!("Check CHANGE_DIR declaration in issue {}", task_issue.html_url)) + .expect("Failed to compile regex"); // Get all of the changed files let pr_files = all_pages("changed files in pull request", octocrab, async || { octocrab From aff7dcf11181749ac662cc76161096908a92c510 Mon Sep 17 00:00:00 2001 From: l Date: Wed, 17 Dec 2025 14:54:07 +0000 Subject: [PATCH 17/28] use option instead of default 0 for issue ids --- src/bin/pr-metadata-validator.rs | 12 ++++++++++-- src/course.rs | 29 ++++++++++++++--------------- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index 8015ab2..e49c3a0 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -251,7 +251,10 @@ async fn validate_pr( return Ok(ValidationResult::BodyTemplateNotFilledOut); } - let pr_assignment_descriptor_id = get_descriptor_id_for_pr(matched.sprints, pr_number); + let pr_assignment_descriptor_id = + get_descriptor_id_for_pr(matched.sprints, pr_number).expect("This PR does not exist"); + // This should never error, as a PR by this point in code must have been matched + // with an assignment, and PR assignments must have an associated issue descriptor match check_pr_file_changes( octocrab, @@ -300,7 +303,12 @@ async fn check_pr_file_changes( None => return Ok(None), // There is no match defined for this task, don't do any more checks }; let directory_matcher = Regex::new(directory_description_regex) - .with_context(|| format!("Check CHANGE_DIR declaration in issue {}", task_issue.html_url)) + .with_context(|| { + format!( + "Check CHANGE_DIR declaration in issue {}", + task_issue.html_url + ) + }) .expect("Failed to compile regex"); // Get all of the changed files let pr_files = all_pages("changed files in pull request", octocrab, async || { diff --git a/src/course.rs b/src/course.rs index 16e8460..da2e374 100644 --- a/src/course.rs +++ b/src/course.rs @@ -314,7 +314,7 @@ pub enum Assignment { ExpectedPullRequest { title: String, html_url: Url, - assignment_descriptor_id: u64, + assignment_issue_id: u64, optionality: AssignmentOptionality, }, } @@ -543,7 +543,7 @@ pub enum Submission { PullRequest { pull_request: Pr, optionality: AssignmentOptionality, - assignment_descriptor: u64, + assignment_issue_id: u64, }, } @@ -944,6 +944,7 @@ fn match_pr_to_assignment( sprint_index: usize, assignment_index: usize, optionality: AssignmentOptionality, + assignment_issue_id: u64, } let mut best_match: Option = None; @@ -964,6 +965,7 @@ fn match_pr_to_assignment( Assignment::ExpectedPullRequest { title: expected_title, optionality, + assignment_issue_id, .. } => { let mut assignment_title_words = make_title_more_matchable(expected_title); @@ -988,6 +990,7 @@ fn match_pr_to_assignment( sprint_index, assignment_index, optionality: optionality.clone(), + assignment_issue_id: *assignment_issue_id, }); } } @@ -1000,22 +1003,15 @@ fn match_pr_to_assignment( sprint_index, assignment_index, optionality, + assignment_issue_id, .. }) = best_match { - let pr_assignment_descriptor_id: u64 = - match assignments[sprint_index].assignments[assignment_index] { - Assignment::ExpectedPullRequest { - assignment_descriptor_id, - .. - } => assignment_descriptor_id, - _ => 0, - }; submissions[sprint_index].submissions[assignment_index] = SubmissionState::Some(Submission::PullRequest { pull_request: pr, optionality, - assignment_descriptor: pr_assignment_descriptor_id, + assignment_issue_id: assignment_issue_id, }); } else if !pr.is_closed { unknown_prs.push(pr); @@ -1024,7 +1020,10 @@ fn match_pr_to_assignment( // Given a vector of sprints, and a target pr number, for a given person // return the issue ID for the associated assignment descriptor -pub fn get_descriptor_id_for_pr(sprints: Vec, target_pr_number: u64) -> u64 { +pub fn get_descriptor_id_for_pr( + sprints: Vec, + target_pr_number: u64, +) -> Option { match sprints .iter() .flat_map(|sprint_with_subs| sprint_with_subs.submissions.clone()) @@ -1037,10 +1036,10 @@ pub fn get_descriptor_id_for_pr(sprints: Vec, target_pr_n _ => false, }) { Some(Submission::PullRequest { - assignment_descriptor, + assignment_issue_id, .. - }) => assignment_descriptor, - _ => 0, + }) => Some(assignment_issue_id), + _ => None, // Was called with a nonexistent PR number, can't find an associated assignment } } From cbf16348f2997c367f1a970d0ae681678c5e1511 Mon Sep 17 00:00:00 2001 From: l Date: Wed, 17 Dec 2025 15:01:14 +0000 Subject: [PATCH 18/28] normalise var names --- src/course.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/course.rs b/src/course.rs index da2e374..b1e5d04 100644 --- a/src/course.rs +++ b/src/course.rs @@ -229,6 +229,7 @@ fn parse_issue(issue: &Issue) -> Result html_url: html_url.clone(), optionality, assignment_descriptor_id: *number, + assignment_issue_id: *number, }), "Codility" => { // TODO: Handle these. From 6b28e2fa88cff286bcf9c9d4923b88f8075556b6 Mon Sep 17 00:00:00 2001 From: l Date: Wed, 17 Dec 2025 15:07:47 +0000 Subject: [PATCH 19/28] normalise var names --- src/course.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/course.rs b/src/course.rs index b1e5d04..bc661cf 100644 --- a/src/course.rs +++ b/src/course.rs @@ -228,7 +228,6 @@ fn parse_issue(issue: &Issue) -> Result title: title.clone(), html_url: html_url.clone(), optionality, - assignment_descriptor_id: *number, assignment_issue_id: *number, }), "Codility" => { From 9649cffba612b86e7dbbb5d867c0adedc377e1c3 Mon Sep 17 00:00:00 2001 From: l Date: Wed, 17 Dec 2025 15:12:56 +0000 Subject: [PATCH 20/28] remove taking ownership from sprint vec iter --- src/bin/pr-metadata-validator.rs | 2 +- src/course.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index e49c3a0..8be2e1d 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -252,7 +252,7 @@ async fn validate_pr( } let pr_assignment_descriptor_id = - get_descriptor_id_for_pr(matched.sprints, pr_number).expect("This PR does not exist"); + get_descriptor_id_for_pr(&matched.sprints, pr_number).expect("This PR does not exist"); // This should never error, as a PR by this point in code must have been matched // with an assignment, and PR assignments must have an associated issue descriptor diff --git a/src/course.rs b/src/course.rs index bc661cf..e5b984a 100644 --- a/src/course.rs +++ b/src/course.rs @@ -1021,12 +1021,12 @@ fn match_pr_to_assignment( // Given a vector of sprints, and a target pr number, for a given person // return the issue ID for the associated assignment descriptor pub fn get_descriptor_id_for_pr( - sprints: Vec, + sprints: &[SprintWithSubmissions], target_pr_number: u64, ) -> Option { match sprints .iter() - .flat_map(|sprint_with_subs| sprint_with_subs.submissions.clone()) + .flat_map(|sprint_with_subs| sprint_with_subs.submissions.iter()) .filter_map(|missing_or_submission| match missing_or_submission { SubmissionState::Some(s) => Some(s), _ => None, @@ -1038,7 +1038,7 @@ pub fn get_descriptor_id_for_pr( Some(Submission::PullRequest { assignment_issue_id, .. - }) => Some(assignment_issue_id), + }) => Some(*assignment_issue_id), _ => None, // Was called with a nonexistent PR number, can't find an associated assignment } } From 96d6560c9020d12bdca0705ef6927d6e845cec03 Mon Sep 17 00:00:00 2001 From: l Date: Wed, 17 Dec 2025 15:23:29 +0000 Subject: [PATCH 21/28] use guards instead of matches where appropriate --- src/bin/pr-metadata-validator.rs | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index 8be2e1d..322b7e4 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -284,24 +284,32 @@ async fn check_pr_file_changes( task_issue_number: u64, ) -> Result, Error> { // Get the Sprint Task's description of expected changes - let task_issue = match octocrab + let Ok(task_issue) = octocrab .issues(org_name, module_name) .get(task_issue_number) .await - { - Ok(iss) => iss, - Err(_) => return Ok(Some(ValidationResult::CouldNotMatch)), // Failed to find the right task + else { + return Ok(Some(ValidationResult::CouldNotMatch)); // Failed to find the right task }; + let task_issue_body = task_issue.body.unwrap_or_default(); + let directory_description = Regex::new("CHANGE_DIR=(.+)\\n").expect("Known good regex failed to compile"); - let directory_description_regex = match directory_description.captures(&task_issue_body) { - Some(capts) => capts - .get(1) - .expect("Regex capture failed to return string match") - .as_str(), // Only allows a single directory for now - None => return Ok(None), // There is no match defined for this task, don't do any more checks + let Some(directory_regex_captures) = directory_description.captures(&task_issue_body) else { + return Ok(None); // There is no match defined for this task, don't do any more checks }; + let directory_description_regex = directory_regex_captures + .get(1) + .with_context(|| { + format!( + "Check CHANGE_DIR declaration in issue {}", + task_issue.html_url + ) + }) + .expect("Regex capture failed to return string match") + .as_str(); // Only allows a single directory for now + let directory_matcher = Regex::new(directory_description_regex) .with_context(|| { format!( @@ -310,6 +318,7 @@ async fn check_pr_file_changes( ) }) .expect("Failed to compile regex"); + // Get all of the changed files let pr_files = all_pages("changed files in pull request", octocrab, async || { octocrab @@ -321,6 +330,7 @@ async fn check_pr_file_changes( if pr_files.is_empty() { return Ok(Some(ValidationResult::NoFiles)); // no files committed } + // check each file and error if one is in unexpected place for pr_file in pr_files { if !directory_matcher.is_match(&pr_file.filename) { @@ -329,6 +339,7 @@ async fn check_pr_file_changes( })); } } + Ok(None) } From 17f8dd06cd82cc2e41851cc3924465f217c0bc50 Mon Sep 17 00:00:00 2001 From: l Date: Wed, 17 Dec 2025 15:25:54 +0000 Subject: [PATCH 22/28] normalise var names --- src/course.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/course.rs b/src/course.rs index e5b984a..b1958da 100644 --- a/src/course.rs +++ b/src/course.rs @@ -1011,7 +1011,7 @@ fn match_pr_to_assignment( SubmissionState::Some(Submission::PullRequest { pull_request: pr, optionality, - assignment_issue_id: assignment_issue_id, + assignment_issue_id, }); } else if !pr.is_closed { unknown_prs.push(pr); From 06ffe542135ea318e7ab0a220226434b55b263ca Mon Sep 17 00:00:00 2001 From: LonMcGregor <3817332+LonMcGregor@users.noreply.github.com> Date: Tue, 23 Dec 2025 10:29:23 +0000 Subject: [PATCH 23/28] Update src/bin/pr-metadata-validator.rs Co-authored-by: Daniel Wagner-Hall --- src/bin/pr-metadata-validator.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index 322b7e4..3aedb92 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -265,14 +265,10 @@ async fn validate_pr( ) .await { - Ok(Some(problem)) => return Ok(problem), - Ok(None) => (), - Err(e) => { - let _ = anyhow!(e); - } + Ok(Some(problem)) => Ok(problem), + Ok(None) => Ok(ValidationResult::Ok), + Err(err) => Err(err), } - - Ok(ValidationResult::Ok) } // Check the changed files in a pull request match what is expected for that sprint task From b4a80a104250aab29eabdabf185f32bb053b09bc Mon Sep 17 00:00:00 2001 From: l Date: Tue, 23 Dec 2025 10:41:47 +0000 Subject: [PATCH 24/28] simplify logic in checking for changes --- src/bin/pr-metadata-validator.rs | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index 82c60f8..0cc8bb0 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -250,19 +250,13 @@ async fn validate_pr( // This should never error, as a PR by this point in code must have been matched // with an assignment, and PR assignments must have an associated issue descriptor - match check_pr_file_changes( + check_pr_file_changes( octocrab, github_org_name, module_name, pr_number, pr_assignment_descriptor_id, - ) - .await - { - Ok(Some(problem)) => Ok(problem), - Ok(None) => Ok(ValidationResult::Ok), - Err(err) => Err(err), - } + ).await } // Check the changed files in a pull request match what is expected for that sprint task @@ -272,14 +266,14 @@ async fn check_pr_file_changes( module_name: &str, pr_number: u64, task_issue_number: u64, -) -> Result, Error> { +) -> Result { // Get the Sprint Task's description of expected changes let Ok(task_issue) = octocrab .issues(org_name, module_name) .get(task_issue_number) .await else { - return Ok(Some(ValidationResult::CouldNotMatch)); // Failed to find the right task + return Ok(ValidationResult::CouldNotMatch); // Failed to find the right task }; let task_issue_body = task_issue.body.unwrap_or_default(); @@ -287,7 +281,7 @@ async fn check_pr_file_changes( let directory_description = Regex::new("CHANGE_DIR=(.+)\\n").expect("Known good regex failed to compile"); let Some(directory_regex_captures) = directory_description.captures(&task_issue_body) else { - return Ok(None); // There is no match defined for this task, don't do any more checks + return Ok(ValidationResult::Ok); // There is no match defined for this task, don't do any more checks }; let directory_description_regex = directory_regex_captures .get(1) @@ -318,19 +312,19 @@ async fn check_pr_file_changes( }) .await?; if pr_files.is_empty() { - return Ok(Some(ValidationResult::NoFiles)); // no files committed + return Ok(ValidationResult::NoFiles); // no files committed } // check each file and error if one is in unexpected place for pr_file in pr_files { if !directory_matcher.is_match(&pr_file.filename) { - return Ok(Some(ValidationResult::WrongFiles { + return Ok(ValidationResult::WrongFiles { expected_files_pattern: directory_description_regex.to_string(), - })); + }); } } - Ok(None) + Ok(ValidationResult::Ok) } struct KnownRegions(BTreeMap<&'static str, Vec<&'static str>>); From 51d98d246b001465246fa8f83e5a14811b25348a Mon Sep 17 00:00:00 2001 From: l Date: Tue, 23 Dec 2025 10:50:17 +0000 Subject: [PATCH 25/28] simplify error handling --- src/bin/pr-metadata-validator.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index 0cc8bb0..ec6869f 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -285,12 +285,6 @@ async fn check_pr_file_changes( }; let directory_description_regex = directory_regex_captures .get(1) - .with_context(|| { - format!( - "Check CHANGE_DIR declaration in issue {}", - task_issue.html_url - ) - }) .expect("Regex capture failed to return string match") .as_str(); // Only allows a single directory for now From 8d9d6e3c6e58c89dea629a47da0e2b677fd4fbf9 Mon Sep 17 00:00:00 2001 From: l Date: Tue, 23 Dec 2025 11:13:42 +0000 Subject: [PATCH 26/28] map result errs into user facing --- src/bin/pr-metadata-validator.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index ec6869f..74dfff8 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -1,6 +1,5 @@ use std::{collections::BTreeMap, process::exit}; -use anyhow::{Context, anyhow}; use chrono::NaiveDate; use indexmap::IndexMap; use maplit::btreemap; @@ -177,7 +176,7 @@ async fn validate_pr( .iter() .find(|pr| pr.number == pr_number) .ok_or_else(|| { - anyhow!( + anyhow::anyhow!( "Failed to find PR {} in list of PRs for module {}", pr_number, module_name @@ -279,7 +278,8 @@ async fn check_pr_file_changes( let task_issue_body = task_issue.body.unwrap_or_default(); let directory_description = - Regex::new("CHANGE_DIR=(.+)\\n").expect("Known good regex failed to compile"); + Regex::new("CHANGE_DIR=(.+)\\n") + .map_err(|err| Error::UserFacing(format!("Known good regex failed to compile: {}", err)))?; let Some(directory_regex_captures) = directory_description.captures(&task_issue_body) else { return Ok(ValidationResult::Ok); // There is no match defined for this task, don't do any more checks }; @@ -289,13 +289,13 @@ async fn check_pr_file_changes( .as_str(); // Only allows a single directory for now let directory_matcher = Regex::new(directory_description_regex) - .with_context(|| { + .map_err(|err| Error::UserFacing( format!( - "Check CHANGE_DIR declaration in issue {}", - task_issue.html_url + "Failed to compile regex from {}, check the CHANGE_DIR declaration: {}", + task_issue.html_url, + err ) - }) - .expect("Failed to compile regex"); + ))?; // Get all of the changed files let pr_files = all_pages("changed files in pull request", octocrab, async || { From cf98009170c18c15511441a71dd2e2411c8169c6 Mon Sep 17 00:00:00 2001 From: l Date: Tue, 23 Dec 2025 11:14:07 +0000 Subject: [PATCH 27/28] format --- src/bin/pr-metadata-validator.rs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index 74dfff8..9132a44 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -255,7 +255,8 @@ async fn validate_pr( module_name, pr_number, pr_assignment_descriptor_id, - ).await + ) + .await } // Check the changed files in a pull request match what is expected for that sprint task @@ -277,8 +278,7 @@ async fn check_pr_file_changes( let task_issue_body = task_issue.body.unwrap_or_default(); - let directory_description = - Regex::new("CHANGE_DIR=(.+)\\n") + let directory_description = Regex::new("CHANGE_DIR=(.+)\\n") .map_err(|err| Error::UserFacing(format!("Known good regex failed to compile: {}", err)))?; let Some(directory_regex_captures) = directory_description.captures(&task_issue_body) else { return Ok(ValidationResult::Ok); // There is no match defined for this task, don't do any more checks @@ -288,14 +288,12 @@ async fn check_pr_file_changes( .expect("Regex capture failed to return string match") .as_str(); // Only allows a single directory for now - let directory_matcher = Regex::new(directory_description_regex) - .map_err(|err| Error::UserFacing( - format!( - "Failed to compile regex from {}, check the CHANGE_DIR declaration: {}", - task_issue.html_url, - err - ) - ))?; + let directory_matcher = Regex::new(directory_description_regex).map_err(|err| { + Error::UserFacing(format!( + "Failed to compile regex from {}, check the CHANGE_DIR declaration: {}", + task_issue.html_url, err + )) + })?; // Get all of the changed files let pr_files = all_pages("changed files in pull request", octocrab, async || { From 8518dde10a902e071492dd85d136d9d682c9873f Mon Sep 17 00:00:00 2001 From: LonMcGregor <3817332+LonMcGregor@users.noreply.github.com> Date: Tue, 23 Dec 2025 11:25:36 +0000 Subject: [PATCH 28/28] Update src/course.rs Co-authored-by: Daniel Wagner-Hall --- src/course.rs | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/course.rs b/src/course.rs index b1af473..997bbca 100644 --- a/src/course.rs +++ b/src/course.rs @@ -1015,23 +1015,18 @@ pub fn get_descriptor_id_for_pr( sprints: &[SprintWithSubmissions], target_pr_number: u64, ) -> Option { - match sprints + sprints .iter() .flat_map(|sprint_with_subs| sprint_with_subs.submissions.iter()) - .filter_map(|missing_or_submission| match missing_or_submission { - SubmissionState::Some(s) => Some(s), + .filter_map(|submission_state| match submission_state { + SubmissionState::Some(Submission::PullRequest { + pull_request, + assignment_issue_id, + .. + }) if pull_request.number == target_pr_number => Some(*assignment_issue_id), _ => None, }) - .find(|submission| match submission { - Submission::PullRequest { pull_request, .. } => pull_request.number == target_pr_number, - _ => false, - }) { - Some(Submission::PullRequest { - assignment_issue_id, - .. - }) => Some(*assignment_issue_id), - _ => None, // Was called with a nonexistent PR number, can't find an associated assignment - } + .next() } fn make_title_more_matchable(title: &str) -> IndexSet {