Pre-Commit

[kchason]

• Provides configuration and documentation for pre-commit
• Configures pre-commit to run in the GitHub Actions

[ajnelson-nist]

I found one concern by trying out pre-commit with the virtual environment built as part of this repository's testing workflow. In this sequence of events:

source tests/venv/bin/activate
pre-commit install
deactivate
rm -rf tests/venv  # NOTE: This happens as part of 'make clean'

I have to manually undo the effects of pre-commit install by running rm .git/hooks/pre-commit. Otherwise, git commit ceases to function. This is a significant usability issue, worth warning folks away from. It looks like this would affect folks who follow development starting with the CI steps (clone, then run make check).

It looks like this PR has no automatic effect on CI. I see no impact on CI, so if I forgot to run pre-commit install, it looks like code could still be committed and merged without black intervening in the Github space. In a similar effect, PR 35 added a test to confirm black formatting, but stopped short of incorporating that test into make check.

So, it looks like this PR adds a beneficial way to guarantee a pre-commit script runs, but only for people who can run pre-commit install into a durable environment. That could be system-level, or anything except the virtual environment built as part of the make-oriented development workflow used in this repository. Is that a fair assessment, @kchason ? If so, the new CONTRIBUTE.md section should be adjusted. (It should be adjusted anyway - there's a missing newline, first patched line in that file.)

These are technical questions that are incidental to the main policy question: Will black be guaranteed to have run on every commit (or at least branch head) of this repository before merging into develop? If we think pre-commit handles this for non-Posix (/non-WSL) folks, and make can handle it for Posix (/WSL) folks, we can move to encoding that policy in a follow-on PR. I do suggest that be a follow-on PR, for one last review from the Adoption Committee.

[kchason]

I haven't yet experienced the venv issue but I'll attempt to reproduce to further troubleshoot.

With regards to the CI, pre-commit exits with an error code of 1 if it does not conform to the defined rule-set so it forces compliance with the black formatting, however the contributor chooses to format the code (with a make process, pre-commit, manual black CLI tool, or manually edited).

[ajnelson-nist]

@kchason I left most of how to re-produce the issue in the Bash snippet above. Please let me know if you want me to script the whole toe-stubbing.

Meanwhile, I thought of another solution that avoids the make clean issue. The problem is supporting these constraints:

• Not installing the tests/venv virtual environment, because it can be reasonably expected to be deleted frequently.
• Not requiring an elevated-permissions call to enable pre-commit outside of the tests/venv environment (e.g. in the system-wide Python installation). So far, the CASE utilities do not require admin/root to install, and I'd strongly prefer to keep that true.

My current thinking is to make a dedicated virtual environment in the repository root, once, and not delete it on make clean calls. In Make style, it can be done by adding this recipe's target as a dependency of all:

.venv-pre-commit/var/built.log:
        rm -rf .venv-pre-commit
        python3 -m venv .venv-pre-commit
        source .venv-pre-commit/bin/activate && pip install pre-commit
        source .venv-pre-commit/bin/activate && pre-commit install
        mkdir -p .venv-pre-commit/var
        touch $@

If this rule is an all dependency, pre-commit will work after one run of make, and still work after make clean. This should only be integrated after a .pre-commit-config.yaml file is tracked.

How do we feel about making a persistent virtual environment?

[ajnelson-nist]

Another behavior I've observed, which is likely to impact make users:

Say a file is edited, but not staged for committing (no git add has been run yet). If the pre-commit hooks are run, either via committing some other file(s) or by running pre-commit run, the edited file is stashed with git stash so it stays out of the way of pre-commit doing its review, and then the stash is "popped", restoring the edited file.

It seems that this temporary removal keeps text editor sessions with that edited file open happy because it happens and then reverts quickly enough that the editor either doesn't have time to complain about losing its open file.

However, the stash push-pop cycle updates the timestamp on the edited file. This will cause an impact on a make user, because any re-run of make later will see that updated timestamp and run its update steps accordingly.

So, overall impact of incorporating pre-commit seems it will include unexpected re-runs of workflows.

[ajnelson-nist]

I've been trying this in a few other repositories. It has some behaviors that induce mildly unpleasant side effects for Make and Vim users related to git stash, but it hasn't caused sufficiently big problems that I'd oppose integrating it here.

I'm going to add a Make step to set up that dedicated pre-commit virtual environment, but otherwise there's only one last matter I'd like to know for this PR:

The version of Black listed in the pre-commit setup is hard-coded. What is the practice you'd suggest for keeping that decently fresh? Should we make a pre-release checklist for pull requests into main, one of the steps being reviewing .pre-commit-config.yaml?

[ajnelson-nist]

@kchason , I added three commits to the pre-commit-black branch on this repository. I don't think I have push permission to your fork, and it's perfectly fine for you to not give me that permission.

If you agree with the three commits, please merge them into your branch so this PR will pick them up.

I still look forward to hearing what you think is practical for handling keeping the hard-coded versions fresh.

[ajnelson-nist]

@kchason , I've pushed a catch-up merge that handles what apparently has become a merge conflict since your branch state. Can you please catch your branch up with the pre-commit-black branch on this repository?

[kchason]

Should be all set now

[ajnelson-nist]

Thanks! I'll merge once CI passes.