feat(xtask): add generate-compat command with quirks and comprehensive tests#36
feat(xtask): add generate-compat command with quirks and comprehensive tests#36EffortlessSteven wants to merge 5 commits intomainfrom
Conversation
- Scan compat/devices/ for YAML device manifests - Aggregate by vendor, tier, capabilities - Generate COMPATIBILITY.md with vendor-grouped tables - Generate compat/matrix.json for machine consumption - Add xtask subcommand registration Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoAdd compat-matrix xtask generator with vendor aggregation
WalkthroughsDescription• Add compat-matrix xtask subcommand to generate compatibility documentation • Parse device and game YAML manifests from compat/ directory structure • Aggregate devices by vendor with tier and capability statistics • Generate vendor-grouped COMPATIBILITY.md with device and game tables • Export machine-readable compat/matrix.json with summary and detailed metrics • Include 5 unit tests for parsing, aggregation, and helper functions Diagramflowchart LR
A["YAML Manifests<br/>devices/ games/"] -->|collect_manifests| B["Parse Device<br/>& Game Files"]
B -->|aggregate| C["Vendor Map<br/>& Statistics"]
C -->|generate| D["COMPATIBILITY.md"]
C -->|serialize| E["compat/matrix.json"]
D -->|output| F["Documentation"]
E -->|output| G["Machine-readable"]
File Changes1. xtask/src/compat_matrix.rs
|
Code Review by Qodo
1.
|
There was a problem hiding this comment.
Pull request overview
This PR adds a new cargo xtask compat-matrix subcommand that generates COMPATIBILITY.md and compat/matrix.json from YAML device and game manifests. Unlike the existing gen-compat/generate-compat commands (in compat.rs), the new command adds vendor-grouped summary tables, capability coverage statistics, and a richer JSON export with vendor aggregations.
Changes:
- New
compat_matrixmodule added asxtask/src/compat_matrix.rs(626 lines), implementing manifest scanning, vendor aggregation, Markdown and JSON generation, and 5 unit tests. xtask/src/main.rsupdated to declare the new module and add theCompatMatrixCLI variant.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
xtask/src/compat_matrix.rs |
New module implementing the compat-matrix command: parses device/game YAML manifests, aggregates by vendor, generates vendor-grouped COMPATIBILITY.md and compat/matrix.json |
xtask/src/main.rs |
Registers the compat_matrix module and adds the CompatMatrix subcommand variant to the CLI enum |
| fn generate_markdown( | ||
| devices: &[MatrixDevice], | ||
| games: &[MatrixGame], | ||
| vendors: &[VendorSummary], | ||
| devices_by_tier: &BTreeMap<String, usize>, | ||
| games_by_tier: &BTreeMap<String, usize>, | ||
| vendor_map: &BTreeMap<String, Vec<MatrixDevice>>, | ||
| ) -> Result<String> { |
There was a problem hiding this comment.
The generate_markdown function accepts both a devices: &[MatrixDevice] slice (used only for computing summary counts at lines 257–266) and a vendor_map: &BTreeMap<String, Vec<MatrixDevice>> (which contains all the same device data). Since summary counts such as total devices, devices with axes, etc. can be derived directly from vendor_map by iterating its values, the devices parameter is redundant and creates confusion about the single source of truth. Consider removing devices and computing summary counts from vendor_map instead.
xtask/src/compat_matrix.rs
Outdated
| fn parse_device(path: &Path) -> Result<MatrixDevice> { | ||
| let text = fs::read_to_string(path)?; | ||
| let doc: serde_yaml::Value = serde_yaml::from_str(&text)?; | ||
|
|
||
| Ok(MatrixDevice { | ||
| name: doc["device"]["name"].as_str().unwrap_or("?").to_string(), | ||
| vendor: doc["device"]["vendor"] | ||
| .as_str() | ||
| .unwrap_or("?") | ||
| .to_string(), | ||
| vendor_id: doc["device"]["usb"]["vendor_id"] | ||
| .as_u64() | ||
| .map_or_else(|| "?".to_string(), |v| format!("0x{v:04X}")), | ||
| product_id: doc["device"]["usb"]["product_id"] | ||
| .as_u64() | ||
| .map_or_else(|| "?".to_string(), |v| format!("0x{v:04X}")), | ||
| axes: doc["capabilities"]["axes"]["count"].as_u64().unwrap_or(0), | ||
| buttons: doc["capabilities"]["buttons"].as_u64().unwrap_or(0), | ||
| force_feedback: doc["capabilities"]["force_feedback"] | ||
| .as_bool() | ||
| .unwrap_or(false), | ||
| tier: doc["support"]["tier"].as_u64().unwrap_or(0), | ||
| test_coverage: DeviceTestCoverage { | ||
| simulated: doc["support"]["test_coverage"]["simulated"] | ||
| .as_bool() | ||
| .unwrap_or(false), | ||
| hil: doc["support"]["test_coverage"]["hil"] | ||
| .as_bool() | ||
| .unwrap_or(false), | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| fn parse_game(path: &Path) -> Result<MatrixGame> { | ||
| let text = fs::read_to_string(path)?; | ||
| let doc: serde_yaml::Value = serde_yaml::from_str(&text)?; | ||
|
|
||
| let control_injection = { | ||
| let ci = &doc["features"]["control_injection"]; | ||
| let std_events = ci["standard_events"].as_bool().unwrap_or(false); | ||
| let direct = ci["direct"].as_bool().unwrap_or(false); | ||
| let dataref = ci["dataref_write"].as_bool().unwrap_or(false); | ||
| let commands = ci["commands"].as_bool().unwrap_or(false); | ||
| std_events || direct || dataref || commands | ||
| }; | ||
|
|
||
| Ok(MatrixGame { | ||
| name: doc["game"]["name"].as_str().unwrap_or("?").to_string(), | ||
| id: doc["game"]["id"].as_str().unwrap_or("?").to_string(), | ||
| mechanism: doc["integration"]["mechanism"] | ||
| .as_str() | ||
| .unwrap_or("?") | ||
| .to_string(), | ||
| crate_name: doc["integration"]["crate"] | ||
| .as_str() | ||
| .unwrap_or("?") | ||
| .to_string(), | ||
| features: GameFeatures { | ||
| telemetry_read: doc["features"]["telemetry_read"] | ||
| .as_bool() | ||
| .unwrap_or(false), | ||
| control_injection, | ||
| force_feedback_translation: doc["features"]["force_feedback_translation"] | ||
| .as_bool() | ||
| .unwrap_or(false), | ||
| aircraft_detection: doc["features"]["aircraft_detection"] | ||
| .as_bool() | ||
| .unwrap_or(false), | ||
| }, | ||
| test_coverage: GameTestCoverage { | ||
| trace_replay: doc["test_coverage"]["trace_replay"] | ||
| .as_bool() | ||
| .unwrap_or(false), | ||
| hil: doc["test_coverage"]["hil"].as_bool().unwrap_or(false), | ||
| }, | ||
| tier: doc["support_tier"].as_u64().unwrap_or(0), | ||
| }) | ||
| } | ||
|
|
||
| // ---------- helpers ---------- | ||
|
|
||
| fn collect_manifests(dir: &Path) -> Result<Vec<PathBuf>> { | ||
| if !dir.exists() { | ||
| return Ok(Vec::new()); | ||
| } | ||
| let mut paths = Vec::new(); | ||
| collect_yaml(dir, &mut paths); | ||
| paths.sort(); | ||
| Ok(paths) | ||
| } | ||
|
|
||
| fn collect_yaml(dir: &Path, out: &mut Vec<PathBuf>) { | ||
| let Ok(entries) = fs::read_dir(dir) else { | ||
| return; | ||
| }; | ||
| for entry in entries.flatten() { | ||
| let p = entry.path(); | ||
| if p.is_dir() { | ||
| collect_yaml(&p, out); | ||
| } else if p.extension().is_some_and(|e| e == "yaml") { | ||
| out.push(p); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn bool_to_check(v: bool) -> &'static str { | ||
| if v { "✓" } else { "✗" } | ||
| } |
There was a problem hiding this comment.
The compat_matrix module duplicates a large amount of logic already present in compat.rs:
parse_deviceis functionally identical (same YAML field paths, same fallbacks).parse_gameis functionally identical.collect_manifests/collect_yamlare byte-for-byte copies.bool_to_checkis a copy.GameFeaturesandGameTestCoverageare structurally identical structs;DeviceTestCoverageis a renamed copy ofTestCoverage.
This means any future change to the YAML schema must be applied in two places. Consider extracting the shared parsing and helper logic into a shared internal module (e.g. compat_shared.rs) that both compat.rs and compat_matrix.rs can reuse.
xtask/src/compat_matrix.rs
Outdated
| .with_context(|| format!("Failed to write {json_path}"))?; | ||
|
|
||
| println!("✓ Written {md_path} ({} bytes)", md.len()); | ||
| println!("✓ Written {json_path} ({} bytes)", json_out.len()); |
There was a problem hiding this comment.
The reported byte count for matrix.json at line 219 (json_out.len()) reflects the serialized JSON string length before the trailing newline is appended on line 215. The file is actually 1 byte larger than reported. This is a minor discrepancy but may be misleading. Use the total written bytes (i.e., json_out.len() + 1) or compute the length from the format string for accuracy.
| println!("✓ Written {json_path} ({} bytes)", json_out.len()); | |
| println!("✓ Written {json_path} ({} bytes)", json_out.len() + 1); |
xtask/src/main.rs
Outdated
|
|
||
| /// Generate COMPATIBILITY.md and compat/matrix.json with vendor-grouped stats | ||
| CompatMatrix, | ||
|
|
There was a problem hiding this comment.
The enum variant GenCompat (line 73) and GenerateCompat (line 76) both already existed and both call compat::run_gen_compat(). The new CompatMatrix (line 79) adds a third way to generate COMPATIBILITY.md. The docstring for GenerateCompat says "Generate COMPATIBILITY.md and compatibility.json from compat/ manifests" while CompatMatrix says "Generate COMPATIBILITY.md and compat/matrix.json with vendor-grouped stats" — both write to COMPATIBILITY.md but produce different content. The Commands enum now has three overlapping compatibility-generation variants. Consider either consolidating them or clearly differentiating their output file names to avoid user confusion.
xtask/src/compat_matrix.rs
Outdated
| let md_path = "COMPATIBILITY.md"; | ||
| fs::write(md_path, &md).with_context(|| format!("Failed to write {md_path}"))?; |
There was a problem hiding this comment.
Both CompatMatrix (new) and GenCompat/GenerateCompat (existing) write to the same COMPATIBILITY.md file. Running one command after the other will silently overwrite the previous output with no indication to the user. Either the new command should write to a distinct filename (e.g. COMPATIBILITY-MATRIX.md), or the docstrings/descriptions should make it explicit that only one command should be used and its file format takes precedence. The PR description says both generate COMPATIBILITY.md, so users have no clear way to know which output to use.
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| let vendors: Vec<VendorSummary> = vendor_map | ||
| .iter() | ||
| .map(|(name, devs)| VendorSummary { | ||
| name: name.clone(), | ||
| device_count: devs.len(), | ||
| tier_1: devs.iter().filter(|d| d.tier == 1).count(), | ||
| tier_2: devs.iter().filter(|d| d.tier == 2).count(), | ||
| tier_3: devs.iter().filter(|d| d.tier == 3).count(), | ||
| ffb_devices: devs.iter().filter(|d| d.force_feedback).count(), | ||
| }) |
There was a problem hiding this comment.
1. Vendor tiers hardcoded 🐞 Bug ✓ Correctness
Per-vendor tier counts and the vendor summary table only handle tiers 1–3, but the repo’s device manifests contain tier 4 and tier 5 devices. This will misreport vendor statistics in both COMPATIBILITY.md and compat/matrix.json (and makes the tier legend incomplete).
Agent Prompt
### Issue description
`compat-matrix` hardcodes per-vendor tier counts to tiers 1–3, but the manifest dataset includes device tiers 4 and 5. This produces misleading vendor stats and an incomplete tier legend.
### Issue Context
- Device manifests already contain `support.tier: 4` and `support.tier: 5`.
- The generator already builds global tier distributions dynamically (`devices_by_tier`), but vendor summaries do not.
### Fix Focus Areas
- xtask/src/compat_matrix.rs[53-61]
- xtask/src/compat_matrix.rs[155-165]
- xtask/src/compat_matrix.rs[277-295]
- xtask/src/compat_matrix.rs[353-367]
### Implementation notes
- Replace `VendorSummary { tier_1, tier_2, tier_3 }` with a dynamic `tiers: BTreeMap<String, usize>` (or `BTreeMap<u64, usize>`) and update JSON + markdown generation accordingly.
- Alternatively, if tiers are known/fixed (1–5), add explicit fields for tier_4/tier_5 and update the vendor table header and legend.
- Update the tier legend section to cover all tiers that appear in data (or clearly state which tiers are defined/unsupported).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
- Use dynamic tier tracking (BTreeMap<u64, usize>) instead of hardcoded 1-3 - Remove redundant devices parameter from generate_markdown - Fix byte count for matrix.json (len + 1 for trailing newline) - Consolidate with existing compat command (remove CompatMatrix variant) - Reuse parsing from compat.rs (shared types and helper functions) - Dynamic vendor table columns and tier legend based on data Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove redundant vendors/devices_by_tier/games_by_tier parameters from generate_markdown; compute summary counts from vendor_map.values().flatten() - CompatMatrix writes to COMPATIBILITY-MATRIX.md to avoid overlap with existing COMPATIBILITY.md generation - Update command descriptions and module docs to reflect new output path Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add xtask/src/generate_compat.rs module that: - Scans compat/devices/ and compat/games/ YAML manifests - Parses device quirks from YAML into the DeviceEntry type - Generates COMPATIBILITY.md sorted by vendor -> name with quirks column - Generates compat/compatibility.json for programmatic consumption - Validates required fields and tier ranges on all manifests - Includes 11 unit tests covering: - Device parser (valid, missing fields, invalid tier) - Game parser (valid, missing features, invalid tier) - Generator output (markdown structure, tier counts, sort order) Wire Commands::GenerateCompat to the new module (gen-compat still uses compat_matrix for backward compatibility). Update CI to use 'cargo xtask generate-compat' and diff both COMPATIBILITY.md and compat/compatibility.json. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
EffortlessSteven
changed the title
Address review comments on PR #36: - Remove redundant devices param from generate_compat::generate_markdown, derive device list and tier counts from vendors map - Extract shared tier-counting helpers (compute_devices_by_tier, compute_games_by_tier) into compat.rs to eliminate duplicated logic - Make support tier legend dynamic instead of hardcoded tiers 1-3, using tier_meaning() helper (supports tiers 1-5 with fallback) - Widen tier validation from 1..=3 to 1..=5 to match manifest reality - Clarify GenCompat vs GenerateCompat command help text to distinguish output filenames (COMPATIBILITY-MATRIX.md vs COMPATIBILITY.md) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Adds \xtask/src/generate_compat.rs\ module implementing \cargo xtask generate-compat\ that generates \COMPATIBILITY.md\ and \compat/compatibility.json\ from the device/game YAML manifests.
Changes
*New module: \generate_compat.rs*
Shared types (\compat.rs)
CI (\ci.yml)
Tests (11 new unit tests)
How to test
\\�ash
cargo xtask generate-compat # Generate outputs
cargo test -p xtask generate_compat # Run the 11 new tests
\\