Address repo-review suggestions

[dcherian]

What is your issue?

Here's the output from the Scientific Python Repo Review tool.

There's an online version here.

On mac I run

pipx run 'sp-repo-review[cli]' --format html --show err gh:pydata/xarray@main | pbcopy

A lot of these seem fairly easy to fix. I'll note that there's a large number of mypy config suggestions.

General

• Detected build backend: setuptools.build_meta
• Detected license(s): Apache Software License

?	Name	Description
❌	PY007	Supports an easy task runner (nox or tox) Projects must have a noxfile.py or tox.ini to encourage new contributors.

PyProject

See https://github.com/pydata/xarray/issues/8239#issuecomment-1739363809

?	Name	Description
❌	PP305	Specifies xfail_strict xfail_strict should be set. You can manually specify if a check should be strict when setting each xfail. [tool.pytest.ini_options] xfail_strict = true
❌	PP308	Specifies useful pytest summary -ra should be in addopts = [...] (print summary of all fails/errors). [tool.pytest.ini_options] addops = ["-ra", "--strict-config", "--strict-markers"]

Pre-commit

?	Name	Description
❌	PC110	Uses black Use https://github.com/psf/black-pre-commit-mirror instead of https://github.com/psf/black in .pre-commit-config.yaml
❌	PC160	Uses codespell Must have https://github.com/codespell-project/codespell repo in .pre-commit-config.yaml
❌	PC170	Uses PyGrep hooks (only needed if RST present) Must have https://github.com/pre-commit/pygrep-hooks repo in .pre-commit-config.yaml
❌	PC180	Uses prettier Must have https://github.com/pre-commit/mirrors-prettier repo in .pre-commit-config.yaml
❌	PC191	Ruff show fixes if fixes enabled If --fix is present, --show-fixes must be too.
❌	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'

MyPy

?	Name	Description
❌	MY101	MyPy strict mode Must have strict in the mypy config. MyPy is best with strict or nearly strict configuration. If you are happy with the strictness of your settings already, ignore this check or set strict = false explicitly. [tool.mypy] strict = true
❌	MY103	MyPy warn unreachable Must have warn_unreachable = true to pass this check. There are occasionally false positives (often due to platform or Python version static checks), so it's okay to ignore this check. But try it first - it can catch real bugs too. [tool.mypy] warn_unreachable = true
❌	MY104	MyPy enables ignore-without-code Must have "ignore-without-code" in enable_error_code = [...]. This will force all skips in your project to include the error code, which makes them more readable, and avoids skipping something unintended. [tool.mypy] enable_error_code = ["ignore-without-code", "redundant-expr", "truthy-bool"]
❌	MY105	MyPy enables redundant-expr Must have "redundant-expr" in enable_error_code = [...]. This helps catch useless lines of code, like checking the same condition twice. [tool.mypy] enable_error_code = ["ignore-without-code", "redundant-expr", "truthy-bool"]
❌	MY106	MyPy enables truthy-bool Must have "truthy-bool" in enable_error_code = []. This catches mistakes in using a value as truthy if it cannot be falsey. [tool.mypy] enable_error_code = ["ignore-without-code", "redundant-expr", "truthy-bool"]

Ruff

?	Name	Description
❌	RF101	Bugbear must be selected Must select the flake8-bugbear B checks. Recommended: [tool.ruff] select = [ "B", # flake8-bugbear ]

[max-sixty]

Most of these seem quite reasonable! And cool that there's an automated review tool.

(One that I would suggest we don't do is mypy strict mode, which is really really strict, and instead pick off some parts of that and gradually apply them through the repo..., e.g. #8198)

[mathause]

I worked on the pytest config in #8246. We cannot follow the suggestions exactly because:

• xfail_strict = true the xfails depend on the environment and installed versions
• -ra is too noisy - it lists all the skipped and xfailed tests. The default of -rfE (report failures and errors) is better for us

[alhridoy]

Hi @dcherian I've reviewed the open issues. Before diving in, I wanted to reach out and confirm if the issue is still relevant and open for contributions. if there are any specific guidelines or best practices you'd like me to follow.

[keewis]

This issue definitely is still up for grabs, with the easiest being the items in the pre-commit hooks section.

I wouldn't recommend doing too much in a single PR: for example, if you were to add the mirrors-prettier hook, that should be its own PR because the new hook might add other changes (making reviews harder than they need to be).

[kmuehlbauer]

One PR per change seems totally reasonable. When finalizing this issue, those commits should also be put in the .git-blame-ignore-revs to make usage of blame easier.

[mathause]

FWIW the codespell is not really recommended as pre-commit hook - see discussion in #6316

edit: better formulation

[dcherian]

Updated to only show errors.