Update readme and tox.ini for recent tooling changes #1868

EliahKagan · 2024-03-12T19:16:45Z

There are some changes that I should have proposed and/or made in README.md and tox.ini related to #1862, which are made slightly more needed by #1865. This brings the readme and tox configuration up to date for both #1862, which was merged, and #1865, which I think will be merged soon ~~pending one small change~~. Further related changes should be eventually be made in both, but they will depend on future decisions, and nothing bad will happen if changes end up not being made for an extended time after merging both #1865 and this PR.

The changes here seem significantly more cumbersome to recommend be included in #1865 than to do separately, and because they also apply to the situation brought about since #1862, I don't think they need to be done there or done before that is merged. I suggest #1865 be merged before this; the changes here will not be all correct until #1865 comes in anyway. #1865 could be merged and then this immediately merged, but I think it is also fine to merge #1865 and request changes here; that is, I think #1865 is reasonable to merge even if this is delayed. Merging them in the other order would also be okay, I think.

The core issue is that some actions that were presented as not changing file contents now change them. Currently on the main branch:

I believe running tox with no arguments is always in practice assumed not to change source code in the working tree. But its lint environment changes source code due to running ruff with --fix through pre-commit. With #1865, this will happen in more cases, but the issue already exists. As noted below and in commit messages, I believe --fix is nonetheless good and should be kept. (When I reviewed #1862 I did not mention this, because I agreed with the change, which is what users of pre-commit will expect. In hindsight, I could've simplified things by proposing these changes at that time.)
The readme documents make lint as linting and running a formatting check without modifying any files. This is likewise not fully accurate since #1862. The issue is not that make lint needs to behave differently, because (a) in hindsight I shouldn't have added that target, (b) I'm not sure anyone uses it, and (c) it was added to address a situation where checking for code style and formatting problems involved multiple commands and tool and plugin packages, instead of just ruff. So it's sufficient for the readme to stop recommending it as a way to avoid linting without automatic code changes.

This PR fixes (1) by making the tox lint environment not run unless explicitly listed on the command line, for now, and fixes (2) by changing the readme. This also fixes some related outdated material in the readme, improves how tools are described, and makes some some other small improvements when it seemed like I could do them without significantly complicating this PR or its review.

Most information about these changes is in the two most important commits, c66257e and 91f967a. This includes some information about why I believe --fix should be kept.

I've also included information in 91f967a about what it does not do and why it does not do it, which may be considered excessive in a commit message, and I'd be pleased to amend that and move that information into this PR description on request.
The reason I've included that information is so that no matter what order things happen in, and even if some are omitted, and regardless of what related changes do or don't happen in the future, and even if I am making a mistake in thinking that it is better to have this additional PR rather than delay and expand the scope of #1865, the full context will still be readily available to anyone who looks at that commit while trying to figure something out.

EliahKagan · 2024-03-12T20:31:49Z

Due to the small README.md changes in 1b8812a recently added to #1865, this PR may end up having a trivial conflict in README.md in its commit c66257e. Although that does not supersede this PR or the other readme changes c66257e makes, I suggest #1865 simply be merged, after which I can rebase this assuming there is a conflict.

- Add a missing period. - Indicate sh rather than bash as as the language for syntax highlighting of shell commands that don't need bash. I had held off on making that second change in previous revisions because it would have involved either introducing an inconsistency or editing the section giving the deprecated signature-checking instructions. That section was removed in 2671167 (gitpython-developers#1823). (This also wraps a paragraph where the immediately surrounding text was wrapped, but that should not affect rendered text, and broader consistency improvements to Markdown wrapping style are not done.)

If people who want to run the tests didn't install the test extra, they can still install that extra. This simplifies the instructions accordingly. test-requirements.txt is still mentioned near the beginning in case people want to look at it to see dependencies. But the changed code is the only place where the instructions had still said to do anything with those files. A possible disadvantage of this change is that in the rare case that someone following those instructions to run the tests locally didn't do an editable installation, then installing with the extra shouldn't be done editably either. But this doesn't seem like a likely problem, and the new text has an example command with -e to clarify the kind of command whose effect is augmented here. Because of that subtlety, it is not obvious that this change is really justified for purposes of clarity. However, this also helps prepare for a time when test-requirements.txt won't exist anymore, as may happen when the project definition is made fully declarative (see discussion in comments in gitpython-developers#1716).

This also reorganizes the "Specific tools" list, since they are all configured in pyproject.toml now (only flake8 was not before, and it was removed in favor of ruff in gitpython-developers#1862). In doing so, I've also added brief parenthesized phrases to characterize what each of these four tools is for, so readers don't have to look around as much to understand most of the tooling GitPython has set up.

This is to make it so simple `tox` usage has the expected property of leaving all source code files in the working tree unchanged. Linting how sometimes performs auto-fixes since gitpython-developers#1862, and the pre-commit command in tox.ini, which had also run `black --check`, will do even more file editing due to the changes in gitpython-developers#1865. The bifurcation for black into separate mutating and non-mutating hooks, introduced in 5d8ddd9 (gitpython-developers#1693), was not carried over into Ruff autoformatting in gitpython-developers#1865. But also it: - Was not necessarily a good approach, and likely should not be preserved in any form. It was an unusual and unintuitive use of pre-commit. (It can be brought back if no better approach is found, though.) - Was done to avoid a situation where it was nontrivial to set up necessary dependencies for linting in the GitPython virtual environment itself, because flake8 and its various plugins would have to be installed. They were not listed in any existing or newly introduced extra (for example, they were not added to test-requirements.txt) in part in the hope that they would all be replaced by Ruff, which happened in gitpython-developers#1862. - Already did not achieve its goal as of gitpython-developers#1862, since it was (probably rightly) not extended to Ruff linting to use/omit --fix. Now that Ruff is being used, people can run `pip install ruff` in a virtual environment, then run the `ruff` command however they like. This takes the place of multiple tools and plugins. The situation with the tox "lint" environment is thus now similar to that of the tox "html" environment when it was added in e6ec6c8 (gitpython-developers#1667), until it was improved in f094909 (gitpython-developers#1693) to run with proper isolation.

EliahKagan · 2024-03-13T06:39:52Z

I've rebased this onto main to get the changes from #1865, and reworded commit messages accordingly, including removing some of the less important text in the last commit message (the situation is simpler now that #1865 is removed, especially since it ended up including the removal of the lint target in Makefile, so that extra information is out of date as well as less important).

Byron

Thanks a lot for these updates and the incredible reasoning and documentation that comes with them.

If all of GitPython would have been done like these, many things would be clearer today I am sure. Thus, I don't think any commit message should be changed or truncated, and you are always welcome to leave as much information as you like and where you like.

EliahKagan · 2024-03-13T13:44:02Z

Thanks. I kept various details; I only shortened those that would have to have been updated to be accurate given the history behind the rebase, which was faster and easier than revising those parts. The uncertainty about what changes were going to be made had been the reason I'd included that extra information, too.

However, I've noticed that the links to a commit in a PR's history (which GitHub automatically causes to be pasted in when copying hashes from the PR page) become quite useless when the PR is rebased so that commit is no longer part of it, no longer leading anywhere the changes may be seen.

I've replaced those links in the PR with the hash prefixes themselves, so the history can be discerned for as long as the otherwise orphaned commits still exist on the remote (or maybe such hashes in a message keep the commits alive? I don't know). Please note that those commits are outdated though--their messages don't reflect the changes in #1865 since my review of it, or that #1865 is in the past, etc.

For anyone interested, the old c66257e and 91f967a correspond to the new 8262343 and b059cd5.

It makes sense for ruff linting and ruff autoformatting to be easily runnable individually and to have their results be shown separately. Splitting them out in tox.ini also makes it so tox can do the other tests corresponding to those in lint.yml on CI in an environment that requries no custom behavior from pre-commit other than skipping the ruff checks. The ruff linting ("ruff") and ruff format checking ("format") tox environments specify ruff as a dependency and call it directly rather than through pre-commit, invoking it in such a way that it does not attempt to modify any files in the working tree. See b059cd5 (gitpython-developers#1868) for context. All three of these tox envs that "lint" has been split into are listed in the env_list and thus run automatically when tox is run with no arguments, since no tox envs unexpectedly (or at all) modify files in the working tree anymore. One limitation of the current approach is that new pre-commit hooks configured in .pre-commit-config.yml will automatically be run as part of the "misc" tox environment, which means that new unexpected mutating operatons could be added if the impact on tox is not considered. The benefit of having it work this way is that most hooks that are likely to be added to GitPython would not modify files and would be wanted as part of "misc". But this may benefit from further refinement.

This is no longer used. No pre-commit hook specifies it anymore in `stages`, since 517f83a (gitpython-developers#1865). See b059cd5 (gitpython-developers#1868) for context. In the lint.yml GitHub Actions workflow, this removes the extra_args key altogether, because all that would remain there is --all-files, which is already the default for that action, when the extra_args key is absent.

It makes sense for ruff linting and ruff autoformatting to be easily runnable individually and to have their results be shown separately. Splitting them out in tox.ini also makes it so tox can do the other tests corresponding to those in lint.yml on CI in an environment that requries no custom behavior from pre-commit other than skipping the ruff checks. The ruff linting ("ruff") and ruff format checking ("format") tox environments specify ruff as a dependency and call it directly rather than through pre-commit, invoking it in such a way that it does not attempt to modify any files in the working tree. See b059cd5 (gitpython-developers#1868) for context. All three of these tox envs that "lint" has been split into are listed in the env_list and thus run automatically when tox is run with no arguments, since no tox envs unexpectedly (or at all) modify files in the working tree anymore. One limitation of the current approach is that new pre-commit hooks configured in .pre-commit-config.yml will automatically be run as part of the "misc" tox environment, which means that new unexpected mutating operatons could be added if the impact on tox is not considered. The benefit of having it work this way is that most hooks that are likely to be added to GitPython would not modify files and would be wanted as part of "misc". But this may benefit from further refinement.

EliahKagan marked this pull request as ready for review March 12, 2024 19:20

EliahKagan mentioned this pull request Mar 12, 2024

lint: switch Black with ruff-format #1865

Merged

EliahKagan added 4 commits March 13, 2024 02:32

EliahKagan force-pushed the lint branch from 91f967a to b059cd5 Compare March 13, 2024 06:38

Byron approved these changes Mar 13, 2024

View reviewed changes

Byron merged commit e880c33 into gitpython-developers:main Mar 13, 2024
26 checks passed

EliahKagan deleted the lint branch March 13, 2024 13:13

EliahKagan mentioned this pull request Mar 13, 2024

Split tox lint env into three envs, all safe #1870

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update readme and tox.ini for recent tooling changes #1868

Update readme and tox.ini for recent tooling changes #1868

EliahKagan commented Mar 12, 2024 •

edited

Loading

EliahKagan commented Mar 12, 2024 •

edited

Loading

EliahKagan commented Mar 13, 2024

Byron left a comment

EliahKagan commented Mar 13, 2024 •

edited

Loading

Update readme and tox.ini for recent tooling changes #1868

Update readme and tox.ini for recent tooling changes #1868

Conversation

EliahKagan commented Mar 12, 2024 • edited Loading

EliahKagan commented Mar 12, 2024 • edited Loading

EliahKagan commented Mar 13, 2024

Byron left a comment

Choose a reason for hiding this comment

EliahKagan commented Mar 13, 2024 • edited Loading

EliahKagan commented Mar 12, 2024 •

edited

Loading

EliahKagan commented Mar 12, 2024 •

edited

Loading

EliahKagan commented Mar 13, 2024 •

edited

Loading