Skip to content

Comments

Add platform-specific variable overrides#17

Merged
james-w merged 2 commits intomainfrom
feat/windows-support
Dec 14, 2025
Merged

Add platform-specific variable overrides#17
james-w merged 2 commits intomainfrom
feat/windows-support

Conversation

@james-w
Copy link
Owner

@james-w james-w commented Dec 13, 2025

Summary

This PR adds platform-specific variable overrides to handle cross-platform differences in commands and configurations.

Feature

Enable platform-specific variable overrides in both [globals] and target-level variables sections to support cross-platform commands.

Syntax:

[globals]
tool = "./tool"

[globals.platform.windows]
tool = "./tool.exe"

[globals.platform.linux]
tool = "./tool"

[globals.platform.macos]
tool = "./tool"

Key Features:

  • Reserved word: platform cannot be used as a variable name
  • Unknown platforms cause deserialization errors
  • Backward compatible: existing configs work unchanged
  • Early resolution: platform overrides resolved during config parsing
  • Empty values supported: allows clearing variables on specific platforms
  • Compile-time platform detection with error on unsupported platforms

Example usage:
See pls.toml for a practical example with the local-install command that uses platform-specific installation commands.

Implementation Details

Core Changes

  • New types: PlatformOverrides and Variables structs in src/config.rs
  • Validation for platform overrides in src/validate.rs
  • Resolution logic in src/context.rs
  • Compile-time platform selection using #[cfg(target_os = "...")]

Validation

  • Only keys must be non-empty; values can be empty strings
  • Supports clearing variables on specific platforms (e.g., tool_args = "")
  • Reserved word "platform" is validated and rejected

Testing

  • 186 tests pass on all platforms (Windows, Linux, macOS)
  • 4 integration tests for platform-specific variables
  • 7 unit tests for validation edge cases
  • 98.12% line coverage in validate.rs

CI Improvements

  • Windows coverage collection enabled (previously excluded)
  • Platform-specific flags for Codecov uploads (Windows, Linux, macOS)
  • Per-platform coverage analysis available on Codecov dashboard

Checklist

  • All tests pass on all platforms
  • CI checks pass (fmt, clippy, check, tests for Windows/Linux/macOS)
  • Feature demonstrated in pls.toml
  • Backward compatible
  • Comprehensive test coverage

🤖 Generated with Claude Code

Enable platform-specific variable overrides in both [globals] and
target-level variables to support cross-platform commands.

Syntax:
- [globals.platform.windows] - Windows overrides for global vars
- [globals.platform.linux] - Linux overrides
- [globals.platform.macos] - macOS overrides
- [command.*.variables.platform.*] - Same for target-level variables

Features:
- Reserved word: 'platform' cannot be used as a variable name
- Unknown platforms cause deserialization errors
- Backward compatible: existing configs work unchanged
- Early resolution: platform overrides resolved during config parsing
- Context only sees resolved HashMap<String, String>

Example usage in pls.toml:
[command.exec.local-install.variables]
install_cmd = "sh -c 'install target/debug/pls ~/.local/bin/pls'"

[command.exec.local-install.variables.platform.windows]
install_cmd = "copy target\debug\pls.exe %USERPROFILE%\.local\bin\pls.exe"

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@james-w james-w force-pushed the feat/windows-support branch from 14c8abe to c6139d1 Compare December 13, 2025 16:39
@codecov
Copy link

codecov bot commented Dec 13, 2025

Codecov Report

❌ Patch coverage is 92.46575% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.80%. Comparing base (6406395) to head (59dd201).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/config.rs 86.84% 0 Missing and 5 partials ⚠️
src/context.rs 73.33% 0 Missing and 4 partials ⚠️
src/validate.rs 97.84% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #17      +/-   ##
==========================================
- Coverage   79.13%   78.80%   -0.33%     
==========================================
  Files          38       39       +1     
  Lines        6359     6742     +383     
  Branches     6359     6742     +383     
==========================================
+ Hits         5032     5313     +281     
- Misses        977     1069      +92     
- Partials      350      360      +10     
Flag Coverage Δ
Linux 79.47% <91.66%> (?)
Windows 72.30% <91.66%> (?)
macOS 71.27% <92.36%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This commit enhances the platform-specific variable overrides feature with
better validation, improved test coverage, and complete CI coverage collection.

## Changes

### Validation improvements (src/validate.rs)
- Allow empty string values in variables (both base and platform overrides)
- Enables clearing variables on specific platforms (e.g., `tool_args = ""`)
- Added 4 unit tests for validation edge cases:
  - Reserved word "platform" rejection
  - Empty key validation for all platform overrides
  - Empty value support verification

### Platform detection simplification (src/config.rs)
- Simplified compile-time platform detection using direct cfg attributes
- Causes compile error on unsupported platforms instead of silent fallback
- Cleaner code with no intermediate string constants or match statements

### Test coverage improvements (tests/test_variables.rs)
- Added 3 integration tests for validation error paths:
  - Empty key validation in global platform overrides
  - Empty value support in platform overrides
  - Empty key validation in target-level platform overrides
- All tests verify actual runtime behavior, not just parsing

### CI coverage enhancements (.github/workflows/cargo.yml)
- Enable Windows coverage collection (previously excluded)
- All three platforms (Windows, Linux, macOS) now generate coverage
- Added platform-specific flags to Codecov uploads for segmented reporting
- Enables per-platform coverage analysis on Codecov dashboard

## Test Results
- 186 tests pass on all platforms
- validate.rs: 98.12% line coverage (+5.89%)
- All CI checks pass (cargo test, clippy, fmt, check)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@claude
Copy link

claude bot commented Dec 14, 2025

Code Review

This PR adds Windows support and platform-specific variable overrides. The implementation is solid with good test coverage. Here are my findings:

Critical Issues

Platform override bug in Variables::resolve() (src/config.rs:36-55)
The cfg-based platform selection will fail on non-supported platforms (e.g., FreeBSD, Android). When none of the #[cfg(...)] match, platform_values is never defined, causing a compile error.

Recommendation: Add a catch-all case or explicitly handle unsupported platforms:

#[cfg(not(any(target_os = "windows", target_os = "linux", target_os = "macos")))]
let platform_values = &None;

Code Quality Issues

Inconsistent error handling for reserved word (src/validate.rs:46-51)
The reserved word "platform" check exists in validation but the error message suggests it's a deserialization error. The test at tests/test_variables.rs:107 expects a serde error, not a validation error, indicating the validation isn't actually preventing the issue.

The problem is that #[serde(flatten)] on values allows "platform" as a key, and serde parses it as a value before validation runs. You'd need #[serde(deny_unknown_fields)] and explicit field names to prevent "platform" being accepted as a variable.

Cloning overhead (multiple locations)

  • src/config.rs:37,50 - Unnecessary clone() calls in resolve()
  • src/config.rs:104-137 - Multiple clones in resolve_target_names_in()

Consider using references or into_iter() where ownership can be transferred.

Test Coverage Concerns

The codecov report shows 70% patch coverage with 24 lines missing. Key gaps:

  • Error paths in Variables::resolve()
  • Platform override edge cases in target_info_from_config()
  • Some validation branches in validate_variables_option()

Minor Issues

  • src/config.rs:40-41: Comment about "compile error" is misleading given the actual bug
  • tests/test_variables.rs:107: Test name test_platform_reserved_word_error suggests validation error but checks for serde error
  • Validation in src/validate.rs:55-64 could be simplified with a helper function or iterator

Positive Notes

  • Clean separation of concerns with the Variables struct
  • Good documentation in PR description and code comments
  • Comprehensive test coverage for the happy path
  • Backward compatibility maintained

Overall: Fix the platform selection bug before merging. Consider addressing the reserved word validation inconsistency.

@james-w james-w changed the title Add Windows support and platform-specific variable overrides Add platform-specific variable overrides Dec 14, 2025
@james-w james-w merged commit 84c330f into main Dec 14, 2025
11 of 12 checks passed
@james-w james-w deleted the feat/windows-support branch December 14, 2025 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant