Generate HTML Landing Page and Add tests for all-ranks-html CLI flag #122
Conversation
Summary: - Generate HTML landing Page - Respect --no-browser in multi-rank mode - Add four new CLI tests covering: - Standard multi-rank HTML generation - Suppressing browser launch (--no-browser) - Mutually-exclusive flag error (--latest + --all-ranks-html) - Empty-directory error path
src/cli.rs
Outdated
| let mut sorted_ranks: Vec<String> = | ||
| rank_logs.iter().map(|(_, rank)| rank.to_string()).collect(); | ||
| sorted_ranks.sort_by(|a, b| { | ||
| a.parse::<u32>() | ||
| .unwrap_or(0) | ||
| .cmp(&b.parse::<u32>().unwrap_or(0)) | ||
| }); | ||
|
|
There was a problem hiding this comment.
rank_logs is a Vec<(std::path::PathBuf, u32)>, where rank is u32 already at the start. So it's very likely that the rank.parse::<u32>() are avoidable.
You need a Vec<String> to pass to generate_multi_rank_html, but do you need it before sorting the ranks?
There was a problem hiding this comment.
Good point, I'll convert to String only when building the list for generate_multi_rank_html. Do you mean like this?
let mut rank_nums: Vec<u32> = rank_logs.iter().map(|(_, rank)| *rank).collect();
rank_nums.sort_unstable();
let sorted_ranks: Vec<String> = rank_nums.iter().map(|r| r.to_string()).collect();
tests/integration_test.rs
Outdated
| use assert_cmd::prelude::*; | ||
| use assert_cmd::Command as AssertCommand; | ||
| use predicates::prelude::*; |
There was a problem hiding this comment.
nit: it's generally a bad practice to import more than what you need, and especially bad practice to import ::*. but prelude in rust is a curated subset and potentially more acceptable.
I personally still don't like it, it increases our chances one day of defining a function name that's already used by those libraries, and get hit with an unexpected compile error. It also makes it harder for people unfamiliar with rust to look at this file, and know where each function is implemented.
I'd favor individually importing functions/classes, especially if we only need a few like <5.
There was a problem hiding this comment.
Found the specific ones, I'll fix it.
use assert_cmd::Command;
use predicates::boolean::PredicateBooleanExt;
use predicates::str;
tests/integration_test.rs
Outdated
| @@ -1,6 +1,11 @@ | |||
| use assert_cmd::prelude::*; | |||
| use assert_cmd::Command as AssertCommand; | |||
There was a problem hiding this comment.
why are we aliasing it? generally it's not a good idea, especially not for standard utilities that many people will know as Command
tests/integration_test.rs
Outdated
| assert!(rank0_index.exists(), "rank 0 index.html should exist"); | ||
| assert!(rank1_index.exists(), "rank 1 index.html should exist"); | ||
| assert!(landing_page.exists(), "toplevel index.html should exist"); |
There was a problem hiding this comment.
this must have been tedious lol, btw you don't always need to provide a custom error message to assert!. The default error message shows something like assertion failed: rank0_index.exists() which is basically your message. it's okay to keep the messages, but they can become a hassle to maintain
Co-authored-by: Simon Fan <[email protected]>
src/lib.rs
Outdated
| @@ -22,6 +22,8 @@ pub mod parsers; | |||
| mod templates; | |||
| mod types; | |||
|
|
|||
| pub use crate::templates::{CSS, TEMPLATE_MULTI_RANK_INDEX, TEMPLATE_QUERY_PARAM_SCRIPT}; | |||
There was a problem hiding this comment.
you don't need this import, we use crate::templates::*; above
|
Would be great to have a screenshot here in the comments to show the rendering. |
|
Summary:
Generate HTML landing Page
Respect --no-browser in multi-rank mode
Add four new CLI tests covering:
Standard multi-rank HTML generation
Suppressing browser launch (--no-browser)
Mutually-exclusive flag error (--latest + --all-ranks-html)
Empty-directory error path