-
Notifications
You must be signed in to change notification settings - Fork 18
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
setup xtask infra for cheatsheets #159
base: main
Are you sure you want to change the base?
Conversation
Deploying ferrous-systems-rust-training with Cloudflare Pages
|
Could we add a few words (here, or in a README) about how the cheatsheets work - to save having to read and understand the source code? |
Also, it needs a rebase on |
e33892a
to
ddd8c18
Compare
|
As `training-material` grows and changes, maintenance could be a nightmare. We basically don't want to be in the business of remember that a certain slide got reordered or moved from one section to another and thus needs changing in all the cheathseets as well. Therefore, this tool seeks to alleviate that with the following workflow: | ||
|
||
* call `cargo xtask make-cheatsheet python` at the root folder | ||
* scrape Markdown headers in `SUMMARY.md` and segment topics by `Rust Fundamentals`, `Applied Rust` and `Advanced Rust` |
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.
Who does this - the user or the tool? Because step 1 in this list is something the user has to do.
|
||
// first arg is the name of the executable; skip it | ||
let args = env::args().skip(1).collect::<Vec<_>>(); | ||
let args = args.iter().map(|arg| &arg[..]).collect::<Vec<_>>(); |
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.
What is this line for? args
is already a Vec<String>
right?
let args = env::args().skip(1).collect::<Vec<_>>(); | ||
let args = args.iter().map(|arg| &arg[..]).collect::<Vec<_>>(); | ||
|
||
let langs = vec!["python", "go", "cpp", "swift", "java", "julia", "c"]; |
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.
Could be an array?
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.
Yup!
let langs = vec!["python", "go", "cpp", "swift", "java", "julia", "c"]; | ||
|
||
if !langs.contains(&args[1]) { | ||
panic!("{} is not a valid language name. \nExpected one of: python, go, cpp, swift, java, julia, c", &args[1]); |
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'd print the lang list in the panic so it doesn't get out of sync.
["test-cheatsheet", lang] => tasks::test_cheatsheet(lang), | ||
_ => { | ||
eprintln!( | ||
"cargo xtask |
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.
Perhaps move to a static HELP_TEXT: &'static str = "..."
so it doesn't confuse the formatting of the match expression.
} | ||
|
||
#[test] | ||
fn test_get_slide_name() { |
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.
Tests should go in a #[cfg(test)] mod test { ... }
panic!("YOUR LAST_HEADER is not part of the text input. CHECK your `SUMMARY.md` for {LAST_HEADER}"); | ||
} | ||
|
||
let first_header = text.find(INITIAL_HEADER).unwrap(); |
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.
You can handle the error here rather than having a separate contains
check which might get isolated from this line.
let Some(first_header) = text.find(INITIAL_HEADER) else {
panic!("Could not find the initial header {INITIAL_HEADER:?}. Check your `SUMMARY.md`");
}
} | ||
|
||
#[test] | ||
fn test_focus_regions() { |
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.
Tests should go in a #[cfg(test)] mod test { ... }
} | ||
|
||
#[test] | ||
fn test_extract_slides() { |
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.
Tests should go in a #[cfg(test)] mod test { ... }
OK, I think this tool makes sense. I had some minor points about the program itself, but I also think we should be running the unit tests in CI, and testing our cheat-sheets in CI. |
Co-authored-by: Jonathan Pallant <[email protected]>
re-assign to me when you're ready, thanks! |
Problem:
Making cheatsheets like "From Python to Rust" and friends is going to be a maintainability nightmare as
training-materials
grows, changes ownership, etc., over time. This was raised as a concern in #148 and this PR seeks to fix that.Proposed solution:
Setup a
xtask
style workflow where wecargo xtask make-cheatsheet python
at the root folderSUMMARY..md
and segment topics byRust Fundamentals
,Applied Rust
andAdvanced Rust
src/python-cheatsheet.md
if it doesn't existpython-cheatsheet.md
are in the appropriate sections, in order, and none are missing.The code is (I think) defensive to spare future pain for someone else looking at this code
(myself in a few months).This PR only sets up the infrastructure but doesn't add any cheatsheets themselves - I'll file a separate PR with a basic Julia cheatsheet once this lands to separate concerns.
Current functionality: