Is code check mandatory?

[biggates]

I'd like to discuss about improving the current code checking process, maybe using pre-commit, which can make the code check process easier.

Current Status

Right now, several tools including isort, black, docformatter, flake8, and pylint are required to run manually, as stated in CONTRIBUTING#Run code checks to keep our code style consistent.

However these checks are NOT mandatory, and running these tools is not easy. so contributors can actually ignore these checks.

e.g. for EVERY FILE edited, one must run the following tools (and setting the correct filename to replace myfile.py)

isort myfile.py -l 120  --balanced --multi-line 3 --lines-between-types 1 --lines-after-imports 2 --trailing-comma
black myfile.py --line-length 120
docformatter myfile.py --wrap-summaries 120 --wrap-descriptions 113 --blank --in-place
flake8 myfile.py --max-line-length=127 --max-complexity=10 --ignore E303,C901,E203,W503
pylint myfile.py --max-line-length=127 --load-plugins=pylint.extensions.docparams --load-plugins=pylint.extensions.docstyle --variable-naming-style=any --argument-naming-style=any --reports=n --suggestion-mode=y --disable=E303 --disable=R0913 --disable=R0801 --disable=C0114 --disable=E203 --disable=E0401 --disable=W9006 --disable=C0330 --disable=R0914 --disable=R0912 --disable=R0915 --disable=W0102 --disable=W0511 --disable=C1801 --disable=C0111 --disable=R1705 --disable=R1720 --disable=C0301 --disable=C0415 --disable=C0103 --disable=C0302 --disable=R1716 --disable=W0632 --disable=E1136 --extension-pkg-whitelist=numpy

There is also a GitHub workflow that runs these tools against ALL files, but it does not provide any errors, so most time people just ignore it.

e.g. : in this workflow run targeted at PR #1097:

black has reformatted 225 files:

reformatted /home/runner/work/NeuroKit/NeuroKit/tests/tests_rsp.py
reformatted /home/runner/work/NeuroKit/NeuroKit/tests/tests_signal.py

All done! ✨ 🍰 ✨
225 files reformatted, 124 files left unchanged.

pylint has several errors:

neurokit2/emg/emg_analyze.py:105:11: E0606: Possibly using variable 'features' before assignment (possibly-used-before-assignment)

However these re-formatted files are not commited back to the PR, and these errors do not block the merge. So after running all these checks, these problems still exists.

Proposed Solution

If we decide to enforce the code check, here are some choices:

1. Improve the current GitHub workflow

   Which keeps everything the same, but in the workflow, commit all the modified files back to PR. So checks are run remotely but enforced.

2a. Or, switch to local pre-commit hooks
Which actually adds a fine-tuned .pre-commit-config.yaml file in the repo, which tells pre-commit to do the exactlly same checks.

Contributors can run pre-commit run to check all the modified files with this yaml file. In addition, it can be executed automatically if one runs pre-commit install locally.

It is best to re-format EVERY .py file with this hook in this step, too. So every commit after this will be focusing on the modified parts.

Documentary work is also needed with this step, to help collaborators correctly set up there hooks.

2b. Automated Checks on Pull Requests
After the first step, we can set up tools like pre-commit.ci to run the checks automatically on every PR, so it is enforced.

3. Switch to ruff for faster checks (Note: May Change Formatting)
   As a future step, we might consider switching some of the code check tools to ruff, which is much faster.

   However, switching to ruff might result in a large number of formatting changes across the codebase, which would require careful planning and coordination.

Let's Discuss

I'd love to hear your thoughts on:

• Whether the code check process should be mandatory.
• Whether we should adopt pre-commit to make code checking process more visible to collaborators.
• Any concerns you might have, maybe moving to ruff.

I am able to provide a PR of pre-commit if this makes sense.

Looking forward to your input!

[DerAndereJohannes]

Thanks for such a detailed account of your thoughts!

I agree that the current solution is not that optimal, since the current pipelines don't really do anything and return a checkmark even if there are problems.

I am very much a fan of making the entire process as effortless as possible for the developer with requiring the least dependencies as possible to setup for them. I am unsure whether using an additional dependency of ruff or pre-commit to be quite large commitments.

Honestly, The optimal solution to me would be some sort of a github bot that applies all the auto-formatting to a pull request if a contributer were to type something like '!format'. I like this because the development of a feature doesn't need to care about reformatting and it allows for a simple solution to run when you think you are ready. This way all the checks would run again before we can push it onto dev. I am unsure of the maintainability of such a tool though (which may be an even larger problem).

I would be happy to hear other thoughts on this too.

[biggates]

I agree with using a bot or GitHub Actions to auto-format code. However, I'd like to raise a few practical concerns for further discussion:

1. Code formatting tools like black or isort, this approach will work very well. A bot or GitHub Action is indeed an ideal solution in these cases.
2. For linters like pylint, which could report issues that require manual fixes:

• I think most problems are just silently ignored currently.
• With locally hooked pre-commit, contributors can see and fix these issues locally BEFORE commiting their changes, so each commit is "clean".
• With only remotely running bots, however, contributors would need to keep checking the bot's output in the PR, pull the bot commits, make fixes and push, and check the PR status again. This process which could be not so friendly.
• Every time a bot makes changes, it creates a new commit (may contain several files). While squash merging ensures the dev branch remains clean, these extra commits may bring more work in reviewing PRs.

So basically pre-commit does these:

1. Make the whole checking process easier (just run pre-commit run instead of multiple tools)
2. (Optionally, only if contributtors installs pre-commit and installs its gits hooks) runs automatically before each commit, locally

It seems that pre-commit ci lite is the bot you want? it works in github actions, but I haven't used that yet.