Skip to content

style: Add extended Ruff rules from Scientific Python repo review#2688

Merged
kratsg merged 11 commits intomainfrom
style/extended-ruff-rules
Apr 3, 2026
Merged

style: Add extended Ruff rules from Scientific Python repo review#2688
kratsg merged 11 commits intomainfrom
style/extended-ruff-rules

Conversation

@kratsg
Copy link
Copy Markdown
Contributor

@kratsg kratsg commented Apr 3, 2026

Pull Request Description

This PR adds the additional extended rules for ruff from scientific-python/cookie: https://github.com/scientific-python/cookie/blob/10e253aaaa7f22418b95d0e446cea216acd4d5a7/%7B%7Bcookiecutter.project_name%7D%7D/pyproject.toml#L284-L329 .

The notebooks and validation code need updating / additional love.

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Add additional Ruff rules recommended by the scientific-python/cookie
  / Scientific Python repo review.
   - Add ARG, BLE, C4, DTZ, EM, EXE, FA, FLY, FURB, G, ICN, ISC, LOG,
     NPY, PD, PERF, PGH, PIE, PL, PT, PTH, PYI, Q, RET, RSE, SIM, SLOT,
     T10, T20, TC, TRY, YTT.
   - Add per-file T20 ignores for notebooks, tests, noxfile, and validation
     scripts where print is legitimate. PLC0415, TC001, TC003 are ignored
     pending further review. PLR09, PLR2004 ignored per convention.
   - Noteable recurring fixes:
      * logging-f-string (G004)
      * raw-string-in-exception (EM101)
      * error-instead-of-exception (TRY400)
   - Amends PR https://github.com/scikit-hep/pyhf/pull/2686

@review-notebook-app
Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@kratsg kratsg requested a review from matthewfeickert April 3, 2026 02:15
@kratsg kratsg self-assigned this Apr 3, 2026
@kratsg kratsg added refactor A code change that neither fixes a bug nor adds a feature packaging setup.py, setup.cfg, pyproject.toml, and friends labels Apr 3, 2026
@github-project-automation github-project-automation bot moved this to In progress in pyhf v0.8.0 Apr 3, 2026
@kratsg kratsg changed the title improve: add extended ruff rules from scientific-pyhon style: add extended ruff rules from scientific-pyhon Apr 3, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 3, 2026

Codecov Report

❌ Patch coverage is 97.43590% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.18%. Comparing base (c0e2dff) to head (a0d1174).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/pyhf/cli/rootio.py 71.42% 2 Missing ⚠️
src/pyhf/parameters/utils.py 66.66% 2 Missing ⚠️
src/pyhf/readxml.py 90.00% 2 Missing ⚠️
src/pyhf/writexml.py 75.00% 2 Missing ⚠️
src/pyhf/contrib/utils.py 95.65% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2688      +/-   ##
==========================================
+ Coverage   98.15%   98.18%   +0.02%     
==========================================
  Files          65       65              
  Lines        4227     4289      +62     
  Branches      472      461      -11     
==========================================
+ Hits         4149     4211      +62     
- Misses         47       48       +1     
+ Partials       31       30       -1     
Flag Coverage Δ
contrib 98.06% <97.43%> (+0.02%) ⬆️
doctest 98.18% <97.43%> (+0.02%) ⬆️
unittests-3.10 96.36% <92.59%> (+<0.01%) ⬆️
unittests-3.11 96.36% <92.59%> (+<0.01%) ⬆️
unittests-3.12 96.36% <92.59%> (+<0.01%) ⬆️
unittests-3.13 96.36% <92.59%> (+<0.01%) ⬆️
unittests-3.9 96.43% <92.87%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

kratsg and others added 8 commits April 3, 2026 11:48
Adopts the scientific-python cookie extended rule set, adding:
ARG, BLE, C4, DTZ, EM, EXE, FA, FLY, FURB, G, ICN, ISC, LOG, NPY,
PD, PERF, PGH, PIE, PL, PT, PTH, PYI, Q, RET, RSE, SIM, SLOT, T10,
T20, TC, TRY, YTT

Per-file T20 ignores for notebooks, tests, noxfile, and validation
scripts where print is legitimate. PLC0415, TC001, TC003 ignored
pending further review. PLR09, PLR2004 ignored per convention.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@matthewfeickert matthewfeickert force-pushed the style/extended-ruff-rules branch from dacab57 to 85c204e Compare April 3, 2026 17:48
Copy link
Copy Markdown
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to review this later given that it is massive and is going to have a lot of repeated things, but Claude Code decided to abandon f-string use for some reason which I do not want to do unless there is a very strong reason for it.

I marked two revision areas but it happens a lot more.


To be clear, I love this PR in general though. :D

@github-project-automation github-project-automation bot moved this from In progress to Review in progress in pyhf v0.8.0 Apr 3, 2026
@matthewfeickert matthewfeickert added style Changes that do not affect the meaning of the code and removed packaging setup.py, setup.cfg, pyproject.toml, and friends labels Apr 3, 2026
@matthewfeickert matthewfeickert changed the title style: add extended ruff rules from scientific-pyhon style: Add extended Ruff rules from Scientific Python dev guide Apr 3, 2026
click.echo("\n".join(file_list))
except AttributeError:
log.error(
log.exception(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this still be an error? What rule is moving it to exception?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kratsg. This is all great. The one thing is that we had previously intentionally kept optimize out of __all__ given user confusion and I think we should keep that.

My suggested changes are probably not the right ones though as I'm assuming that Ruff introduced them all.

Copy link
Copy Markdown
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed my suggested revisions, but please review and revert them if you don't like them. Other than that I'm happy. 👍

This will be a great PR for moving forward contributions where there's small stuff that Ruff can handle for us now!

@matthewfeickert matthewfeickert changed the title style: Add extended Ruff rules from Scientific Python dev guide style: Add extended Ruff rules from Scientific Python repo review Apr 3, 2026
@matthewfeickert
Copy link
Copy Markdown
Member

Repo review on this PR's branch:

uvx sp-repo-review[cli] --format html

👍

Repo review:

General

  • Detected build backend: hatchling.build
  • SPDX license expression: Apache-2.0
  • Python requires: >=3.9

?NameDescription
PY001 Has a pyproject.toml
PY002 Has a README.(md|rst) file
PY003 Has a LICENSE* file
PY004 Has docs folder
PY005 Has tests folder
PY006 Has pre-commit config
PY007 Supports an easy task runner (nox, tox, pixi, etc.)

PyProject

?NameDescription
PP002 Has a proper build-system table
PP003 Does not list wheel as a build-dep
PP004 Does not upper cap Python requires
PP005 Using SPDX project.license should not use deprecated trove classifiers
PP006 The dev dependency group should be defined
PP301 Has pytest in pyproject
PP302 Sets a minimum pytest to at least 6 or 9
PP303 Sets the test paths
PP304 Sets the log level in pytest
PP305 Specifies strict xfail
PP306 Specifies strict config
PP307 Specifies strict markers
PP308 Specifies useful pytest summary
PP309 Filter warnings specified

GitHub Actions

?NameDescription
GH100 Has GitHub Actions config
GH101 Has nice names
GH102 Auto-cancel on repeated PRs
GH103 At least one workflow with manual dispatch trigger
GH104 Use unique names for upload-artifact
GH105 Use Trusted Publishing instead of token-based publishing on PyPI
GH200 Maintained by Dependabot
GH210 Maintains the GitHub action versions with Dependabot
GH211 Do not pin core actions as major versions
GH212 Require GHA update grouping

Pre-commit

?NameDescription
PC100 Has pre-commit-hooks
PC110 Uses black or ruff-format
PC111 Uses blacken-docs
PC140 Uses a type checker
PC160 Uses a spell checker
PC170 Uses PyGrep hooks (only needed if rST present)
PC180 Uses a markdown formatter
Must have one of https://github.com/davidanson/markdownlint-cli2, https://github.com/hukkin/mdformat, https://github.com/rbubley/mirrors-prettier, https://github.com/rvben/rumdl-pre-commit in .pre-commit-config.yaml
PC190 Uses a linter (Ruff/Flake8)
PC191 Ruff show fixes if fixes enabled
PC192 Ruff uses `ruff-check` instead of `ruff` (legacy)
PC901 Custom pre-commit CI update message
PC902 Custom pre-commit CI autofix message
PC903 Specified pre-commit CI schedule

MyPy

?NameDescription
MY100 Uses MyPy (pyproject config)
MY101 MyPy strict mode
MY102 MyPy show_error_codes deprecated
MY103 MyPy warn unreachable
MY104 MyPy enables ignore-without-code
MY105 MyPy enables redundant-expr
MY106 MyPy enables truthy-bool

Ruff

All mentioned rules selected

?NameDescription
RF001 Has Ruff config
RF002 Target version must be set
RF003 src directory doesn't need to be specified anymore (0.6+)
RF101 Bugbear must be selected
RF102 isort must be selected
RF103 pyupgrade must be selected
RF201 Avoid using deprecated config settings
RF202 Use (new) lint config section

ReadTheDocs

?NameDescription
RTD100 Uses ReadTheDocs (pyproject config)
RTD101 You have to set the RTD version number to 2
RTD102 You have to set the RTD build image
RTD103 You have to set the RTD python version
RTD104 You have to specify a build configuration now for readthedocs.

Nox

?NameDescription
NOX101 Sets minimum nox version
Set a minimum nox version:

nox.needs_version = "2025.10.14"
NOX102 Sets venv backend
NOX103 Set default per session instead of session list
You should use default= in each session instead of setting a global list.
NOX201 Set a script block with dependencies in your noxfile
You should have a script block with nox in it, for example:

# /// script
# dependencies = ["nox"]
# ///
NOX202 Has a shebang line
You should have a shebang line at the top of your noxfile.py, for example:

#!/usr/bin/env -S uv run --script
NOX203 Provide a main block to run nox
You should have a main block at the bottom of your noxfile.py, for example:

if __name__ == "__main__":
    nox.main()

@kratsg kratsg merged commit ebf841e into main Apr 3, 2026
22 of 23 checks passed
@kratsg kratsg deleted the style/extended-ruff-rules branch April 3, 2026 23:16
@github-project-automation github-project-automation bot moved this from Review in progress to Done in pyhf v0.8.0 Apr 3, 2026
matthewfeickert pushed a commit that referenced this pull request Apr 7, 2026
* Apply Ruff rules ARG001, F401, F811, F821, F841, NPY002, PERF401,
  PTH123, SIM115 for notebooks.
* Amends PR #2688.
@kratsg kratsg mentioned this pull request Apr 8, 2026
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor A code change that neither fixes a bug nor adds a feature style Changes that do not affect the meaning of the code

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants