Skip to content
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

chore: fix windows specific script errors #5051

Merged
merged 3 commits into from
Feb 7, 2025

Conversation

dy0gu
Copy link

@dy0gu dy0gu commented Feb 7, 2025

Summary

This PR aims to solve the errors thrown by just l and just _touch on Windows systems.

just l

The linter succeeds in CI but not locally on my Windows machine. This is because it seems to not lint code with the #[cfg(target_os = "windows")] directive as the runner OS is Linux. In the future I suggest using a matrix runner in CI that runs the same linter workflow on both Windows and Unix so every OS is covered.

These were the fixes I had to implement on my local Windows machine, as recommend by the linter:

  • Removed unnecessary explicit references (&)
  • Added #[cfg(not(windows))] to Linux only variables, avoiding unused variable errors
  • Wrote hexadecimal with equally sized groups of digits

just _touch

Made the script call powershell explicitly, this allows the user to use either CMD, Powershell, or any other shell of their choice on Windows and still have the command called by Powershell under the hood.

Fixes other scripts by extension, like test-lintrule and test-transform, that relied on just _touch to function correctly.

Test Plan

Changes made do not require tests.

@github-actions github-actions bot added the A-CLI Area: CLI label Feb 7, 2025
@dy0gu dy0gu changed the title chore: fix windows lint errors chore: fix windows command errors Feb 7, 2025
@dy0gu dy0gu changed the title chore: fix windows command errors chore: fix windows specific script errors Feb 7, 2025
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you ❤️

@ematipico ematipico merged commit 141f858 into biomejs:next Feb 7, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants