suggest to run codespell before creating PRs.

[fujitatomoya]

follow up of #3263

#3263 (comment) tells that user should be able to know how to run the codespell beforehand creating pull requests, so that they can fix the typos before github workflow runs. this is missing in #3263, so this PR addresses it.

Note

This also should be cherri-picked after #3274 for dev branch.

[ethouris]

Well, I don't think this must be so complicated. All I did was execution of "pip install" and then I just ran the codespell app. I think it would be better to describe these steps for venv as optional in case when there are any problems.

Much more important is to have some script running the codespell with predefined directories and configuration - so that users can run it with no trouble. I'll try to propose something. The main problem is that it should only regard directories that are on the repository and skip whatever private directories, such as shadow build.

[ethouris]

How about this:

Can be added as script/spellcheck. This can be also expanded, for example, to handle selected files, automatic installation of spellcheck etc.

#!/bin/bash

if [[ -z `type -p codespell` ]]; then
	echo >&2 "You need 'codespell' app to run the spell check."
	echo >&2 "Follow instructions in CONTRIBUTING.md document"
	exit 1
fi

function showhelp()
{
	echo "Usage: `basename $0` <workmode> <targets>"
	echo ""
	echo "WORK MODES:"
	echo "  * show [default] - only show spelling errors"
	echo "  * fix - write spelling errors directly to files"
	echo "  * check - like fix, but ask user to confirm ambiguous fix"
	echo "  * review - like fix, but ask user for every modification"
	echo "TARGETS:"
    echo "  * all [default] - check all files in the repository (including index)"
    echo "  * changed - check only files currently modified in the repository"
}


# Check if the script is run inside the repository view
GIT_TOP=`git rev-parse --show-toplevel` || (echo >&2 "Please run this script in the SRT repository directory"; exit)

# Forcefully enter the top repo dir; this is the only directory where it should be run
cd $GIT_TOP

WORKMODE=${1:-show}
EXTRACTION=${2:-all}

CS_OPTIONS=

case $WORKMODE in
	help)
		showhelp
		exit
		;;
	show) 
		;;

	fix)
		CS_OPTIONS="-w "
		;;

	check)
		CS_OPTIONS="-w -i 2"
		;;

	review)
		CS_OPTIONS="-w -i 1"
		;;

	*)
		echo >&2 "Unknown mode. Use 'help' to list available options."
		exit 1
		;;
esac

CS_FILES=

if [[ $EXTRACTION == "all" ]]; then
	CS_FILES="git ls-files"
elif [[ $EXTRACTION == "changed" ]]; then
	CS_FILES="git ls-files -m"
else
	echo >&2 "Unknown extractopn spec '$EXTRACTION'. Use 'all' or 'changed'"
	exit 1
fi

# Unfortunately this isn't Tcl, so we need to do it "space safe" way.
declare -a FILELIST
eval FILELIST=( $($CS_FILES | awk "{print \"'\" \$1 \"'\"}") )

if [[ -z ${FILELIST[@]} ]]; then
	echo "SPELLCHECK: no files listed, not checking."
	exit 0
fi

#echo Running in files from $CS_FILE: $FILELIST

codespell --config scripts/codespell/codespell.cfg $CS_OPTIONS ${FILELIST[@]}

[fujitatomoya]

@ethouris thank you very much for being responsive and constructive 👍

i got a couple of questions against your suggestion.

All I did was execution of "pip install" and then I just ran the codespell app. I think it would be better to describe these steps for venv as optional in case when there are any problems.

that is because you happen to be using older ubuntu distro?

please see https://peps.python.org/pep-0668/, which has been enabled default on ubuntu:lunar or later.
so we are not able to install or up(down)grade packages unless --break-system-packages on those platforms.
--break-system-packages is not recommended because it might break the user environment, that is the original purpose of this PEP.

i believe using venv is the appropriate procedure here without breaking the user system, unless we suggest container environment in this repository such as devcontainer...

Can be added as script/spellcheck. This can be also expanded, for example, to handle selected files, automatic installation of spellcheck etc.

IMO those subcommands can be recommended into codespell project instead? (btw it prints the typos and suggested change with file path with line number.) i would say having this script for multiple platforms including windows are overkill? i would recommend to have one-line command codespell --config scripts/codespell/codespell.cfg that works for all platforms.

and why do we need to extract the changed files? we already make sure the spellcheck via github workflow, so there is only possibility that changed files could generate the spellcheck failure? what use case do you see?

[ethouris]

@ethouris thank you very much for being responsive and constructive 👍

i got a couple of questions against your suggestion.

All I did was execution of "pip install" and then I just ran the codespell app. I think it would be better to describe these steps for venv as optional in case when there are any problems.

that is because you happen to be using older ubuntu distro?

Rather because I don't use Ubuntu at all (unless I need to repro some problems reported by users, which can be only done there), but I like the way you think. ;)

Seriously - I still think these steps can be described as optional, even though inevitable in the latest version of Ubuntu. OTOH I think this is important to be described separately because this torn-off environment shall be used not only for installation, but for running, too. Am I right?

Can be added as script/spellcheck. This can be also expanded, for example, to handle selected files, automatic installation of spellcheck etc.

IMO those subcommands can be recommended into codespell project instead? (btw it prints the typos and suggested change with file path with line number.) i would say having this script for multiple platforms including windows are overkill? i would recommend to have one-line command codespell --config scripts/codespell/codespell.cfg that works for all platforms.

Frankly, if I wanted this to be usable on Windows, I'd provide a separate script written in PowerShell. Or maybe one script written in Tcl that could be used platform-independently.

and why do we need to extract the changed files? we already make sure the spellcheck via github workflow, so there is only possibility that changed files could generate the spellcheck failure? what use case do you see?

I left this as possible useful alternative because users need not be responsible for spelling errors of the files they didn't change. Maybe it's an overkill, especially that running codespell on all files in the repository is taking very little time. Still, this is an optional mode on demand - default is to check all files in the repository. This is actually an important function because we don't want spellcheck to be run in the shadow build directory the users create in the top repo directory.

[fujitatomoya]

I still think these steps can be described as optional, even though inevitable in the latest version of Ubuntu

agree, this can be really optional developers documentation.

OTOH I think this is important to be described separately because this torn-off environment shall be used not only for installation, but for running, too. Am I right?

Yup, i do think so too.

Frankly, if I wanted this to be usable on Windows, I'd provide a separate script written in PowerShell. Or maybe one script written in Tcl that could be used platform-independently.

hmmm, this is hard for me 😓 almost never used windows and powershell...

because users need not be responsible for spelling errors of the files they didn't change.

this is so true.

This is actually an important function because we don't want spellcheck to be run in the shadow build directory the users create in the top repo directory.

i see your concern.

after all, i am not against having the script.

this could be connected with #3265 (comment).
let me have some time to bring up devcontainer idea, to show you how it works.

[fujitatomoya]

@ethouris it would be really appreciated, if you could give me feedback on #3277 to replace this.

[ethouris]

But #3277 doesn't seem to replace this. It adds necessary things to run codespell on many other platforms, but that's a separate things - might be a dependent PR at best, but not a replacement.

[fujitatomoya]

i mean moving into the working directory can be prerequisite, right? if that is no, the scripts also do not work because user can run the script anywhere instead of this working directory. i am maybe missing your requirement and point here, what is the requirement that you have in mind?

[ethouris]

The script I pasted has a check to see if it was run from anywhere inside the repository directory, and it gets to that repo toplevel directory, unless not found.

[fujitatomoya]

okay, now i see what you want here.

btw, how about build process like cmake and configure, seems like build procedure does not have this requirement, but only for other tools? like spellcheck.

[ethouris]

The build is done in the directory where you need it - may be the repo toplevel, but it can be also a separate directory being a shadow build this way. It's a different situation. For cmake you also use an external tool to run it and only the location of the configuration file matters. The build files - the CMakeLists.txt file, as well as the convenience wrapper scripts, configure itself and configure-data.tcl config file - are by definition placed in the toplevel directory, while all other support tools are in scripts. That's why I added the location detection, as users may run it directly from there or try from the toplevel directory. I wouldn't like to bind it to the build system - the build system itself is too complicated already.

[fujitatomoya]

okay got it. thanks for the explanation.

this is separate issue from devcontainer. and i will come up with script, probably borrowing some of your suggestion later.