-
Notifications
You must be signed in to change notification settings - Fork 497
test: exec the codespell executable consistently #3267
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
base: main
Are you sure you want to change the base?
test: exec the codespell executable consistently #3267
Conversation
|
To do anything with codespell, you do need to install it as described in Development Setup: Also, from Calling pytest through
Please don't misunderstand, I do not mean this change is incorrect and shouldn't be applied. Simply, I'd rather everything is as "standard" as possible compared to other packages and the relevant Python documentation (setuptools, pytest). How do other Python packages compare to codespell? |
I missed it, sorry. In my case, as an example, I have
See https://github.com/pypa/flit/blob/main/tests/test_command.py as an example. However, pypa packages are not consistent. |
|
Then isn't the PATH incorrect? A typical PATH looks like this: Ubuntu 22.04$ cat /etc/skel/.profile
# ~/.profile: executed by the command interpreter for login shells.
# This file is not read by bash(1), if ~/.bash_profile or ~/.bash_login
# exists.
# see /usr/share/doc/bash/examples/startup-files for examples.
# the files are located in the bash-doc package.
# the default umask is set in /etc/profile; for setting the umask
# for ssh logins, install and configure the libpam-umask package.
#umask 022
# if running bash
if [ -n "$BASH_VERSION" ]; then
# include .bashrc if it exists
if [ -f "$HOME/.bashrc" ]; then
. "$HOME/.bashrc"
fi
fi
# set PATH so it includes user's private bin if it exists
if [ -d "$HOME/bin" ] ; then
PATH="$HOME/bin:$PATH"
fi
# set PATH so it includes user's private bin if it exists
if [ -d "$HOME/.local/bin" ] ; then
PATH="$HOME/.local/bin:$PATH"
fi
$
$ echo $PATH
/home/username/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/snap/bin
$ |
No. It is the usual What I mean with unsafe is that some users may forget to install the package again, after updating the source code. This is not a problem when doing CI with github, but it is a problem for a normal user since there is no support for automation (unless I missed it again). If you want to keep the current status, maybe you should use tox. |
|
See Development Mode (a.k.a. “Editable Installs”) in the Setuptools documentation. If you install in editable mode, you do not need to reinstall the package. Updates are taken into account instantly. After a regular installation, if you forget to reinstall after updating the source code, of course the last changes won't be taken into account. However, that's the whole purpose of a regular installation, and a user error. Why try to work around a user error? I am more interested in tox, or perhaps nox. Repo-Review suggests:
Not sure which of tox or nox one should use nowadays when starting from scratch:
|
|
@DimitriPapadopoulos, I'm fine with the current state, since it is documented. As for |
be77fde to
122f744
Compare
|
Fixed incorrect types in Additionally, I also confirmed that using Fixed an unused import. |
Currently, in test_basic.py, the run_codespell and run_codespell_stdin functions exec the codespell script from PATH, not the one from the current directory. This means that the codespell_lib package MUST first be installed, before running pytest. The current behavior is unsafe, since an user running pytest in the global environment may get an error or, worse, may actually try to test an old version. Additionally this behavior is not documented in README.md. Update the run_codespell and run_codespell_stdin functions to exec the codespell script via `python -m codespell_lib`. This change will ensure that python will try to search the __main__ module from the current directory first. Copy the codespell_lib directory to the cwd directory, and configure codespell to ignore it using the `-S` option, in order to make the environment clean. Ensure that the cwd parameter in run_codespell and run_codespell_stdin is never None, since the codespell script must not be executed from the current directory. For consistency, also make the args parameter required.
122f744 to
d3e25cc
Compare
Currently, in
test_basic.py, therun_codespellandrun_codespell_stdinfunctions exec the codespell script from PATH, not the one from the current directory. This means that thecodespell_libpackage MUST first be installed, before runningpytest.The current behavior is unsafe, since an user running
pytestin the global environment may get an error or, worse, may actually try to test an old version. Additionally this behavior is not documented inREADME.md.Update the
run_codespellandrun_codespell_stdinfunctions to exec thecodespellscript viapython -m codespell_lib. This change will ensure that python will try to search the__main__module from the current directory first. Copy thecodespell_libdirectory to thecwddirectory, and configurecodespellto ignore it using the-Soption, in order to make the environment clean.Ensure that the
cwdparameter inrun_codespellandrun_codespell_stdinis neverNone, since thecodespellscript must not be executed from the current directory. For consistency, also make theargsparameter required.TODO
Probably there should be some comments explaining why it is necessary to copy the
codespell_libdirectory to thecwddirectory