Apply Scientific Python repo-review suggestions

[DimitriPapadopoulos]

Apply those rules that make sense:
https://learn.scientific-python.org/development/guides/repo-review/?repo=jaraco%2Fskeleton&branch=main

Suggestions:

GH102: Auto-cancel on repeated PRs
At least one workflow should auto-cancel.

concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: true

PC100: Has pre-commit-hooks
Must have https://github.com/pre-commit/pre-commit-hooks repo in .pre-commit-config.yaml

PC901: Custom pre-commit CI message
Should have something like this in .pre-commit-config.yaml:

ci:
  autoupdate_commit_msg: 'chore: update pre-commit hooks'

RF101: Bugbear must be selected
Must select the flake8-bugbear B checks. Recommended:

[tool.ruff.lint]
extend-select = [
  "B",  # flake8-bugbear
]

RF102: isort must be selected
Must select the isort I checks. Recommended:

[tool.ruff.lint]
extend-select = [
  "I",  # isort
]

RF103: pyupgrade must be selected
Must select the pyupgrade UP checks. Recommended:

[tool.ruff.lint]
extend-select = [
  "UP",  # pyupgrade
]

[jaraco]

Thanks for sharing and distilling the results. I'll go through them one by one.

GH102: Auto-cancel on repeated PRs
At least one workflow should auto-cancel.

On this one, I'm at -0. If it's best practice to have this config, perhaps GitHub should make it the default. I see setuptools has adopted it, but with an additional ref_type factor, added in pypa/setuptools#3116. That issue does look generally relevant. Given that code has been stable since, my guess is the Setuptools approach would be suitable to apply here.

I've had other users propose to disable fail-fast, which is kind-of the opposite of this change.

I find it a little frustrating that the authors of GH102 don't explain why I should override the presumably sensible defaults that GitHub provides. I'd really like to see GH102 rewritten to include the motivation (what's the value in tweaking the configuration) and either a feature request or guidance from GitHub that this setting is preferred. It should probably be updated too to recommend a tweak that doesn't break releases.

[jaraco]

PC100: Has pre-commit-hooks

I've previously explored enabling pre-commit, but I've found it adds too much toil and noise to a project. The project is insistent on pinning and frequently updating the config, which at the very least results in lots of nuisance PRs and commits in the history. I've stumbled on a few projects that are using pre-commit, and especially for the stable ones, the commits in the repo are littered with pre-commit bumps. I absolutely don't want to adopt a process that's unwilling to go with the flow and work at head.

See also #109.

[jaraco]

RF101: Bugbear must be selected
RF102: isort must be selected
RF103: pyupgrade must be selected

These sound good. I was tempted to enable isort when migrating from flake8/black, but I wanted to wait for the projects to stabilize, which they basically are now (little risk of having to roll back the migration). There are still probably many projects that haven't yet been updated to support the code style tweaks in ruff, but that's fine. Enabling these settings will cause a lot of toil on the downstream projects, but I'm willing to do it, especially if we can devise a routine to apply ruff --fix after rolling out the setting to downstream projects.

[DimitriPapadopoulos]

I'm not fond of pre-commit myself 😄

Do you have a complete list of downstream projects that use this skeleton? I have already started applying B and UP rules in some of them. Not all rules can be enforced with ruff check --fix or even --unsafe-fix automatically.

About GH102, I must admit I don't understand the gory details of GitHub jobs. Yet, I think concurrency groups could be enabled and fast-fail disabled at the same time:

• We probably want to avoid concurrent workflows (groups of jobs) running on multiple commits, where the latter commit supersedes the former commit. Typically, anyone would want to run a workflow on the last commit and cancel previous instances of the workflow running on superseded commits. However, I think I need to read more of the documentation to get it right.
• At the same time, we can disable fail-fast so that within a running workflow, all jobs complete.

[jaraco]

I experimented adding "I", "B", and "UP" to ruff.toml in the keyring project. It revealed 73 errors of which all but 17 could be mechanically fixed (all but 33 safely):

 keyring main @ git diff
diff --git a/ruff.toml b/ruff.toml
index 7061298..ee3511b 100644
--- a/ruff.toml
+++ b/ruff.toml
@@ -2,6 +2,9 @@
 extend-select = [
        "C901",
        "W",
+       "I",
+       "B",
+       "UP",
 ]
 ignore = [
        # https://docs.astral.sh/ruff/formatter/#conflicting-lint-rules
 keyring main @ ruff --fix .
warning: `ruff <path>` is deprecated. Use `ruff check <path>` instead.
conftest.py:15:9: B018 Found useless attribute access. Either assign it to a variable or remove it.
keyring/backend.py:22:9: UP007 Use `X | Y` for type annotations
keyring/backend.py:69:13: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/backend.py:74:14: UP006 Use `type` instead of `typing.Type` for type annotation
keyring/backend.py:75:17: UP006 Use `type` instead of `typing.Type` for type annotation
keyring/backend.py:97:60: UP007 Use `X | Y` for type annotations
keyring/backend.py:127:19: UP007 Use `X | Y` for type annotations
keyring/backend.py:128:10: UP007 Use `X | Y` for type annotations
keyring/backend.py:146:25: UP006 Use `tuple` instead of `typing.Tuple` for type annotation
keyring/backend.py:151:23: UP006 Use `tuple` instead of `typing.Tuple` for type annotation
keyring/backend.py:214:26: UP006 Use `list` instead of `typing.List` for type annotation
keyring/backend.py:249:39: UP007 Use `X | Y` for type annotations
keyring/backend.py:250:10: UP006 Use `dict` instead of `typing.Dict` for type annotation
keyring/backends/SecretService.py:35:13: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/backends/SecretService.py:48:13: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
keyring/backends/SecretService.py:63:13: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
keyring/backends/Windows.py:14:9: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/backends/Windows.py:21:9: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/backends/kwallet.py:46:13: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
keyring/backends/kwallet.py:99:13: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
keyring/backends/macOS/__init__.py:20:13: B028 No explicit `stacklevel` keyword argument found
keyring/backends/macOS/__init__.py:51:13: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
keyring/backends/macOS/__init__.py:53:13: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
keyring/backends/macOS/__init__.py:65:13: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
keyring/backends/macOS/__init__.py:67:13: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
keyring/backends/macOS/__init__.py:77:13: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
keyring/core.py:137:5: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/http.py:32:22: UP031 Use format specifiers instead of percent format
keyring/testing/backend.py:178:13: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/testing/util.py:69:9: B007 Loop control variable `i` not used within loop body
tests/backends/test_kwallet.py:50:13: UP031 Use format specifiers instead of percent format
tests/backends/test_kwallet.py:61:13: UP031 Use format specifiers instead of percent format
tests/backends/test_kwallet.py:66:13: UP031 Use format specifiers instead of percent format
Found 74 errors (41 fixed, 33 remaining).
No fixes available (16 hidden fixes can be enabled with the `--unsafe-fixes` option).
 keyring main @ ruff --fix --unsafe-fixes .
warning: `ruff <path>` is deprecated. Use `ruff check <path>` instead.
conftest.py:15:9: B018 Found useless attribute access. Either assign it to a variable or remove it.
keyring/backend.py:69:13: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/backends/SecretService.py:35:13: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/backends/SecretService.py:48:13: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
keyring/backends/SecretService.py:63:13: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
keyring/backends/Windows.py:14:9: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/backends/Windows.py:21:9: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/backends/kwallet.py:46:13: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
keyring/backends/kwallet.py:99:13: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
keyring/backends/macOS/__init__.py:20:13: B028 No explicit `stacklevel` keyword argument found
keyring/backends/macOS/__init__.py:51:13: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
keyring/backends/macOS/__init__.py:53:13: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
keyring/backends/macOS/__init__.py:65:13: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
keyring/backends/macOS/__init__.py:67:13: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
keyring/backends/macOS/__init__.py:77:13: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
keyring/core.py:137:5: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/testing/backend.py:178:13: B018 Found useless expression. Either assign it to a variable or remove it.
Found 35 errors (18 fixed, 17 remaining).

After applying the fixes, there were new failures in the tests, including "needs formatting" and mypy failures... so that's annoying. It doesn't appear there's a way to --fix and format in the same invocation (astral-sh/ruff#8232). It does seem that the unfixable issues are all bugbear, so maybe that's an indication to skip bugbear for now. Skipping bugbear, the check + format results in this diff, causing mypy failure:

diff --git a/keyring/_compat.py b/keyring/_compat.py
index 46868a8..7d7c8fa 100644
--- a/keyring/_compat.py
+++ b/keyring/_compat.py
@@ -4,4 +4,6 @@ __all__ = ['properties']
 try:
     from jaraco.classes import properties  # pragma: no-cover
 except ImportError:
-    from . import _properties_compat as properties  # type: ignore[no-redef] # pragma: no-cover
+    from . import (
+        _properties_compat as properties,  # type: ignore[no-redef] # pragma: no-cover
+    )

That's a little discouraging, suggesting there's a lot of toil ahead.

Do you have a complete list of downstream projects that use this skeleton?

I have a list of projects I contribute to, but that's a superset of projects I maintain and only a subset of projects utilizing skeleton. The Coherent OSS page has a curated list of projects. That's probably a pretty good representation of projects dependent on skeleton that I (and others in Coherent OSS) maintain. For projects others maintain, probably the best way to find those would be to search GitHub for non-fork projects containing the badge (note that the year changes, so you'd want to search without the year). @bswck do you have any other advice on how to find skeleton-based projects (based on the work you've done)?

[DimitriPapadopoulos]

Yet B are often the most interesting rules. For example, they detect tests that contain a = b instead of assert a == b. This error happens more frequently than I would have thought.

[jaraco]

I've filed astral-sh/ruff#10516 to track ruff isort fixes breaking mypy tests.

[bswck]

Do you have a complete list of downstream projects that use this skeleton?

I have a list of projects I contribute to, but that's a superset of projects I maintain and only a subset of projects utilizing skeleton. The Coherent OSS page has a curated list of projects. That's probably a pretty good representation of projects dependent on skeleton that I (and others in Coherent OSS) maintain. For projects others maintain, probably the best way to find those would be to search GitHub for non-fork projects containing the badge (note that the year changes, so you'd want to search without the year). @bswck do you have any other advice on how to find skeleton-based projects (based on the work you've done)?

I'll provide a script for it. We could include it in your article later.

[jaraco]

I did some work this evening to make jaraco.develop.projects-run more flexible, and was able to run this command:

 @ env PROJECTS_LIST_URL=https://raw.githubusercontent.com/jaraco/dotfiles/main/projects.txt pip-run 'jaraco.develop>=8.9.1' -- -m jaraco.develop.projects-run -t 'not fork' -- ruff check --select I,B,UP 2>1 | gist
https://gist.github.com/jaraco/066032d456c816caef90ed2563cce1ec

output gist.

Only ~3300 lints to clean up.

Next, I'll see if I can use this routine to enact some of the automatic fixes.

[jaraco]

I've spent some time cleaning up those projects so at least they're building at HEAD. I've also spent some time fixing some of these checks in the projects and learned a few things:

• B007 can lead to false positives but also can point out unused variables.
• B008 can also produce false positives and make the code noisier. In some cases, annotating the parameter as immutable is an effective workaround. In other cases, I've disabled it.
• B015 caught a couple of bugs \o/.
• B018 had a lot of false positives, especially in keyring libraries that use the side effect of attribute access to detect behavior.
• B904 is a nuisance and found everywhere. Usually the solution is to add as err and from err, but it's annoying to fix. It seems to me that the proper fix should be to make this behavior the default in Python.
• B028 is also a nuisance and found often. I'd say about half the time, an explicit stacklevel is helpful. The other half of the time, the caller's frame is not relevant. Adding the default as an explicit parameter works around the issue, but passing the default as a static parameter is not a good practice.

(prior to an edit, I had B904 and B028 swapped above)

If we're going to adopt bugbear checks, I'd suggest to disable B028 and B904 to start.

[DimitriPapadopoulos]

• B015 caught a couple of bugs \o/.

Indeed, I have caught many missing assert in tests with this one.

• B018 had a lot of false positives, especially in keyring libraries that use the side effect of attribute access to detect behavior.

That's especially true of tests. Perhaps disable in tests?

• B028 is a nuisance and found everywhere. Usually the solution is to add as err and from err, but it's annoying to fix. It seems to me that the proper fix should be to make this behavior the default in Python.

Adding from err changes the wording from:

During handling of the above exception, another exception occurred:

to:

The above exception was the direct cause of the following exception:

I doubt the default behaviour will ever change in Python now that that distinction has been introduced. Is doing the "right thing" such a nuisance? On the other hand, is the difference in wording that important and useful?

[jaraco]

I doubt the default behaviour will ever change in Python now that that distinction has been introduced. Is doing the "right thing" such a nuisance? On the other hand, is the difference in wording that important and useful?

It's four elements to two lines, so no, it's not horrible, but it does start to litter the codebase. Especially when by my estimation 99% of the time, if one is explicitly raising an exception in except block, it's a direct cause (compare that with a coding error that raises an exception).

If Python had the intention for every piece of code everywhere to choose between from None and from err, it really would have been nice for the most common form to be streamlined syntactically.

Oh, but I see now, there are in fact three different modes: no indication, from err, and from None... as the latter resets the exception stack.

I still contend, if Python recommends for everyone to write from err, they should make that the default behavior, and instead make another syntax from Any or similar, so that a programmer types nothing for the preferred behavior and types an option for the uncommon case.

I realize bugbear doesn't speak for the whole community, but it's annoying to be adding all of these changes and persistent noise to all the code bases for such little value.

[jaraco]

Here's what B028 looks like, and here's what B904 looks like across the projects.

[Avasam]

With UP, you might want to disable UP038 though astral-sh/ruff#7871

As a sidenote, for projects relying on both setuptools and distutils, (like setuptools itself) you can add them to https://docs.astral.sh/ruff/settings/#lint_isort_extra-standard-library, https://docs.astral.sh/ruff/settings/#lint_isort_known-third-party, https://docs.astral.sh/ruff/settings/#lint_isort_known-first-party, or https://docs.astral.sh/ruff/settings/#lint_isort_known-local-folder such that setuptools is grouped to always be imported before distutils. (not necessarily something to add in skeleton, but a consideration you can have whenever you hit that issue of import order mattering across a codebase)