-
Notifications
You must be signed in to change notification settings - Fork 56
automate the launch of a new cfp with the cfp command #309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
(authored with AI assistance)
Ah interesting, I didn’t fix the TOML correctly in #307 then. |
|
||
Many of the tasks in `src/admin` are automated via a utility called `cargo rpg`. | ||
|
||
The sources for `cargo rpg` as well as the plugin for the mdbook processor are found in crates located in the `crates` directory. The `crates/README.md` summarizes the role of each crate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this file did anything actually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then maybe we can remove it?
Apparently not. But our AI overlords found the problem. |
let capitalized_timeframe = format!("{}H{}", &timeframe[0..4], &timeframe[5..]); | ||
let new_section = format!( | ||
"\n## Next goal period ({})\n\n\ | ||
The next goal period will be {}, running from {} 1 to {} 30. \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next goal period will be {}, running from {} 1 to {} 30. \ | |
The next goal period will be {}, running from the start of {} to the end of {}. \ |
It's probably not always a 30 day month...?
// Found an existing section for this specific timeframe | ||
// Find the end of this section (next section or end of file) | ||
if let Some(next_section_pos) = content[this_period_match.end()..].find("\n## ") { | ||
let end_pos = this_period_match.start() + next_section_pos + this_period_match.end() - this_period_match.start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this also looks weird
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yeah, let me test out that scenario. Maybe I can ask it to generate some unit tests for this code.
@lqd ok, it has some unit tets now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests were helpful in that I could try to focus on them, rather than on the (somewhat peculiar) code itself.
To be clear, I don't really mind landing this as is (with at least the test fixes) -- mostly because it can be cleaned up later, and it seems to work as we expect.
Did you also mean to add the 2025h2 content in this PR as well? 8e4d600
let result = process_readme_content(content, "2026h1", "2026h1"); | ||
|
||
assert!(result.contains("## Next goal period (2026H1)")); | ||
assert!(result.contains("running from January 1 to June 30")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Evidently we don't run these tests on CI. Some of them mention "January 1" but you've since changed to "the start of January".
assert!(result.contains("running from January 1 to June 30")); | |
assert!(result.contains("running from the start of January to the end of June")); |
and a few other times below.
let content = "# Summary\n\n[Introduction](./README.md)\n\n# ⏳ 2025H1 goal process\n\n- [Overview](./2025h1/README.md)\n"; | ||
let result = process_summary_content(content, "2026h1", "2026h1"); | ||
|
||
assert!(result.contains("# ⏳ 2025H1 goal process")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No big deal but I was slightly confused by this, and incorrectly thought it updated the existing section. Maybe adding an assertion that it does not is enough.
assert!(result.contains("- [Overview](./2025h1/README.md)"));
pub fn process_template_content(content: &str, timeframe: &str, lowercase_timeframe: &str) -> String { | ||
// Remove note sections (starting with '> **NOTE:**' and continuing until a non-'>' line) | ||
// But preserve other content starting with '>' | ||
let lines: Vec<&str> = content.lines().collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example of things we can fix later. The loop should use the iterator directly.
let mut new_content = content.to_string(); | ||
|
||
// Create the new section content with capitalized H | ||
let capitalized_timeframe = format!("{}H{}", &timeframe[0..4], &timeframe[5..]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another example of things we can fix later. This is already semantically the timeframe
parameter, especially when we pass the lowercase version as well.
let _year = &timeframe[0..4]; | ||
let half = &timeframe[4..].to_lowercase(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another one for the _year
. And there's lowercase_timeframe
for the half
.
// Found an existing "Next goal period" section | ||
// Find the end of this section (next section or end of file) | ||
if let Some(next_section_pos) = content[next_period_match.end()..].find("\n## ") { | ||
let end_pos = next_period_match.start() + next_section_pos + next_period_match.end() - next_period_match.start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here as well
|
||
// Join and process placeholders | ||
cleaned_lines.join("\n") | ||
.replace("YYYYHN", timeframe) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This is a note for the things we can fix that is discussed elsewhere: to me this is correct, but this function assumes timeframe
is uppercase while the others do not)
println!("Dry run mode - no changes will be made"); | ||
} | ||
// Validate the timeframe format | ||
validate_timeframe(timeframe)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By ensuring/making timeframe
the uppercase version of the goal period, the conversion can be removed from the 3 functions that do it, and make the 1 function assuming it's already the case, correct.
fn test_process_summary_content_no_existing_section() { | ||
// Test adding a new section when no timeframe sections exist | ||
let content = "# Summary\n\n[Introduction](./README.md)\n"; | ||
let result = process_summary_content(content, "2026h1", "2026h1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me this, and in the other tests that follow, should be
let result = process_summary_content(content, "2026h1", "2026h1"); | |
let result = process_summary_content(content, "2026H1", "2026h1"); |
like the first test did.
|
||
Many of the tasks in `src/admin` are automated via a utility called `cargo rpg`. | ||
|
||
The sources for `cargo rpg` as well as the plugin for the mdbook processor are found in crates located in the `crates` directory. The `crates/README.md` summarizes the role of each crate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then maybe we can remove it?
This PR creates a
cfp
command for automating some of the manual steps to preparing for 2025h2.The code itself was generated with the use of Cline. I've tested it but I admit I didn't read it all that closely. I noticed some parts were not written as I'd likely do them but... it works.
Rendered