Skip to content

Add shell script for spell check#11289

Closed
fanyang-mono wants to merge 1 commit intoAzure:mainfrom
fanyang-mono:add_spell_check_shell_script
Closed

Add shell script for spell check#11289
fanyang-mono wants to merge 1 commit intoAzure:mainfrom
fanyang-mono:add_spell_check_shell_script

Conversation

@fanyang-mono
Copy link
Contributor

Add a shell script for non-windows developers and documentation for the scripts.

Copilot AI review requested due to automatic review settings July 22, 2025 16:53
@fanyang-mono fanyang-mono requested a review from a team as a code owner July 22, 2025 16:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a bash version of the existing PowerShell spell check script to support non-Windows developers, along with comprehensive documentation for both scripts.

  • Implements a bash equivalent (invoke-cspell.sh) of the existing PowerShell spell check functionality
  • Adds detailed documentation (README.md) explaining how to use both scripts and configure the spell checker
  • Provides cross-platform spell checking capability for the repository

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
eng/common/spelling/invoke-cspell.sh New bash script implementing cspell invocation with parameter parsing, npm dependency management, and cleanup
eng/common/spelling/README.md Comprehensive documentation covering script usage, configuration, and word dictionary management

# The "files" list must always contain a file which exists, is not empty, and is
# not excluded in ignorePaths. In this case it will be a file with the contents
# "1" (no spelling errors will be detected)
NOT_EXCLUDED_FILE="$SPELL_CHECK_ROOT/$(uuidgen 2>/dev/null || echo "temp_$(date +%s)_$$")"
Copy link

Copilot AI Jul 22, 2025

Choose a reason for hiding this comment

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

The fallback logic for generating a unique filename could fail if date command fails. Consider using a more robust approach like mktemp which is specifically designed for creating temporary files: NOT_EXCLUDED_FILE="$(mktemp "$SPELL_CHECK_ROOT/temp_XXXXXX")"

Suggested change
NOT_EXCLUDED_FILE="$SPELL_CHECK_ROOT/$(uuidgen 2>/dev/null || echo "temp_$(date +%s)_$$")"
NOT_EXCLUDED_FILE="$(mktemp "$SPELL_CHECK_ROOT/temp_XXXXXX")"

Copilot uses AI. Check for mistakes.

# Create a temporary modified config using jq
# Convert SCAN_GLOBS array to JSON array
SCAN_GLOBS_JSON=$(printf '%s\n' "${SCAN_GLOBS[@]}" | jq -R . | jq -s .)
Copy link

Copilot AI Jul 22, 2025

Choose a reason for hiding this comment

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

The script uses jq but doesn't check if it's available. If jq is not installed, this will fail with an unclear error. Add a check similar to the npm check: if ! command -v jq &> /dev/null; then log_error "Could not locate jq. Install jq https://stedolan.github.io/jq/download/"; exit 1; fi

Copilot uses AI. Check for mistakes.

log_info "Setting config in: $CONFIG_PATH"

# Backup original config and set the modified one
Copy link

Copilot AI Jul 22, 2025

Choose a reason for hiding this comment

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

Directly overwriting the config file without backing it up first is risky. If the script is interrupted between this line and the cleanup function, the original config could be lost. Consider creating a backup file first: cp "$CONFIG_PATH" "$CONFIG_PATH.backup" and restore from the backup in cleanup.

Suggested change
# Backup original config and set the modified one
# Backup original config and set the modified one
# Backup the original config file
cp "$CONFIG_PATH" "$CONFIG_PATH.backup"
# Write the modified config

Copilot uses AI. Check for mistakes.
@azure-sdk
Copy link
Collaborator

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

@weshaggard
Copy link
Member

@fanyang-mono what is the context for this? In what context can you not run the existing ps script? We generally try to avoid maintaining copies of these scripts.

Copy link
Member

@weshaggard weshaggard left a comment

Choose a reason for hiding this comment

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

I would like to avoid adding this shell script if possible.

@fanyang-mono
Copy link
Contributor Author

fanyang-mono commented Jul 22, 2025

Hi @weshaggard, I usually develop on mac and would like to have a shell script to run spell checks. Developers working on non-windows platforms usually don't have powershell installed in their system.

Regarding to maintenance, with the help of copilot, I think the human effort needed here can be very minimal. For example, someone make a change to one of the script, they could simply prompt to copilot "apply the same change to <the other scrip>".

@benbp
Copy link
Member

benbp commented Jul 22, 2025

I appreciate the effort to improve the cross platform dev experience! Most of our engsys/developer automation requires powershell core as a base dependency, precisely because it allows us to write scripts with cross platform support instead of duplicating logic. Powershell is a very easy tool to install on any platform. I have been gradually making our powershell scripts executable from a bash shell by running chmod +x <script> and adding a shebang to the first line: #! /bin/env pwsh (example). That way you can just do ./eng/common/spelling/Invoke-Cspell.ps1 without having to drop into pwsh from bash. I think it would be better to go with that approach here as well.

@fanyang-mono
Copy link
Contributor Author

@benbp Thanks for acknowledge my intention here. I appreciate that you are aware of the non-windows dev experience. I agree that it is easy to install powershell. However, I can't help but wonder why installing powershell when there is built-in bash and if powershell is the best choice for cross-platform scripting.

@benbp
Copy link
Member

benbp commented Jul 22, 2025

@fanyang-mono it's a fair question, and a debate we've had quite a bit on the team. The large majority of azure sdk contributors work on windows, with a fair amount on linux/wsl and a few on mac. Powershell has tended to be a better common denominator than bash for this (windows installations are typically Git Bash which is a poor substitute), and it's been an easier scripting language to handle at scale than bash (easily 10k+ lines across the repos). There's also the consideration that common functions in powershell are going to be more consistent in implementation across platforms and we can enforce a minimum runtime version vs. calling out to core utils (gnu on linux vs. freebsd on mac or whatever version of bash or gnu coreutils the user may have installed).

We've also considered/debated python, js and other languages to handle this but have come back to powershell mainly because it's simpler to shell out and easier for simple ad-hoc scripting.

I acknowledge that it's not necessarily the most popular choice, but it's the one we've made and thus a convention we try to stick to. Arguably it's better to have one consistent automation language, even if inferior in some ways, than a proliferation of different ones as we are a small team that has to maintain the whole surface area at the end of the day.

@fanyang-mono
Copy link
Contributor Author

By no means that I intend to make it harder for your team. And I respect your team's decision. In terms of which programming language is easier to work with, arguably, it comes down to a personal choice. With that, I am going to withdraw my PR.

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.

5 participants