chore: bump ruff to 0.15.14 and pyright to 1.1.410#2539
Conversation
Picks up new ruff lint rules (RUF052/061/067/069/071/075, D420) and a
new pyright deprecation around `@contextmanager` return types. Notable
changes beyond the routine auto-fixes:
- Ignore RUF067 globally: `ops/__init__.py`, `ops/_private/__init__.py`,
and `ops/lib/__init__.py` intentionally execute code at module level
(main wrapper, tracer, legacy library loader).
- Ignore RUF069 in `test/*`: round-trip assertions on exact-literal
config values; `pytest.approx` is not appropriate.
- `ops/framework.py` `_event_context`: wrap `yield` in try/finally so
backend `_hook_is_running` and `_event_name` are restored even when
the wrapped block raises.
- `ops/model.py` `_build_destpath`: replace `os.path.commonprefix` with
`Path.is_relative_to` / `relative_to`. Ruff's suggested swap to
`commonpath` would raise `ValueError` on non-matching paths instead
of returning the project's `RuntimeError`.
- `testing/tests/test_charm_spec_autoload.py`: same try/finally fix for
the `import_name` fixture, and switch `Iterator` to `Generator` for
`@contextmanager` per the new pyright deprecation.
- `test/test_jujuversion.py`: use `mock.patch.dict(os.environ, clear=True)`
instead of `mock.patch('os.environ', new={})` so pyright can infer
the dict element type.
The ruff `--preview` formatter now reformats Python code blocks inside
markdown, which accounts for the doc churn.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Ruff's `--preview` formatter rewrites Python code blocks inside markdown files. With `format.quote-style = "single"` set globally that also converted double quotes to single in doc examples, which we want to keep as double quotes. Exclude markdown from the default `ruff format` pass and add a second pass in `tox -e format` / `tox -e lint` that targets markdown only with `format.quote-style = "preserve"`. Code blocks still get reformatted (line wrapping, blank lines, etc.); only the quote-style choice is left to whatever the source already uses. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
| ruff check --preview --fix | ||
| # Second pass over markdown: format Python code blocks but preserve their | ||
| # existing quote style so doc examples keep their double quotes. | ||
| ruff format --preview --config "format.quote-style = \"preserve\"" --config "format.exclude = [\"**/*.py\"]" . |
There was a problem hiding this comment.
Instead of "preserve", we could go with "double" but I think we want to keep the hand-crafted choice maybe?
There was a problem hiding this comment.
We should preserve initially to keep this PR sane to review, but a separate PR standardising either way would be fine with me.
That would let us drop the whole second ruff format invocation, which would be nice. In the meantime, a comment here and below as to why the second invocation is necessary (in addition to the one in pyproject.toml) would be helpful, unless you plan to do a quick follow-up to standardise.
There was a problem hiding this comment.
Is the existing comment not clear enough? I could repeat it below if needed.
I'm not sure we can standardise. I believe we are firmly one the single-quote path in our own code, but not pushing everyone else to it. So the profiles are quote quotes for example, which means the tutorials are, etc.
There was a problem hiding this comment.
Is the existing comment not clear enough? I could repeat it below if needed.
I didn't understand what was going on with the second run without referring back to the pyproject.toml change and the PR description.
I'm not sure we can standardise. I believe we are firmly one the single-quote path in our own code, but not pushing everyone else to it. So the profiles are quote quotes for example, which means the tutorials are, etc.
That split seemed reasonable to me, but now that it's introducing tooling funkiness it seems worth reconsidering (separately from this PR).
There was a problem hiding this comment.
We already exclude the example charms from repo ruff. Presumably we could exclude docs/tutorial too. Then let ruff change the remaining docs to single quotes. Maybe odd for the tutorial to use a different style, but overall things would be more consistent that what we have now. And the tooling setup would be nice enough.
There was a problem hiding this comment.
I've excluded docs/tutorial and removed the double run. It's a bit of a shame to lose the checks against the tutorial blocks, but the majority of them are duplicated from code that is listed elsewhere, so not too bad, and does simplify the setup.
This does increase the number of changes quite a bit (both the quote change and the line length one). Do you think it's too noisy for later blame usage? I could do a separate PR with only those changes and add it to the blame ignore list after merging, and then this one would be much smaller.
There was a problem hiding this comment.
If James has a strong opinion I'm happy to follow that. But I don't mind reviewing the quote change + line length change in this PR.
To answer a different thread - yes, 80 looks good to me for the docs line length. Thanks!
I was assuming that we'd go ahead with using literalinclude in the tutorial, but we hadn't necessary decided that, had we. (My bad.) Anyway, I think excluding the tutorial for now is good. We can always unexclude it later if we end up taking a different direction from literalinclude.
Take ruff's RUF071 auto-fix suggestion (component-based commonpath) instead of swapping in Path.is_relative_to / relative_to. `commonpath` is a minimal drop-in for the original `commonprefix` call: same shape, just component-based rather than character-by-character, so the partial-component false-match RUF071 warns about can't happen. `is_relative_to` is a purely lexical check that's widely misunderstood; the docs recommend `prefix == p or prefix in p.parents` for the same job, but the simpler choice here is to stay close to the original code. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
james-garner-canonical
left a comment
james-garner-canonical
left a comment
There was a problem hiding this comment.
Thanks for this. There are a couple of cases where we should override the formatter's decisions: most universally, some of our docs examples are intentionally indented so you can copy and paste them to a class body (which perhaps can just be framed with a class <actual class name>: in the example. Also some examples I've suggested manually reformatting to better preserve their intent.
| ruff check --preview --fix | ||
| # Second pass over markdown: format Python code blocks but preserve their | ||
| # existing quote style so doc examples keep their double quotes. | ||
| ruff format --preview --config "format.quote-style = \"preserve\"" --config "format.exclude = [\"**/*.py\"]" . |
There was a problem hiding this comment.
We should preserve initially to keep this PR sane to review, but a separate PR standardising either way would be fine with me.
That would let us drop the whole second ruff format invocation, which would be nice. In the meantime, a comment here and below as to why the second invocation is necessary (in addition to the one in pyproject.toml) would be helpful, unless you plan to do a quick follow-up to standardise.
Co-authored-by: James Garner <james.garner@canonical.com>
Move RUF067 from the global ignore list to per-file ignores for the three intentional offenders (ops/__init__.py, ops/_private/__init__.py, ops/lib/__init__.py) so the rule continues to apply everywhere else. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Framework.observe requires types.MethodType and rejects lambdas, so
the popen example in the hooks migration guide could never have run as
written. Convert the four event bindings to dedicated _on_* methods,
fix the main() invocation, and drop the now-redundant warning bullet
from the {important} callout.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Per review feedback, exclude docs/tutorial/ from ruff entirely (the tutorial will be migrated to literalinclude separately in canonical#2544) and drop the second 'quote-style = preserve' ruff format pass. The remaining docs now use single quotes consistently with the rest of the project. Also add docs/ruff.toml with line-length = 80 so code snippets in the docs wrap at a width that doesn't require horizontal scrolling when rendered. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
james-garner-canonical
left a comment
james-garner-canonical
left a comment
There was a problem hiding this comment.
Single quotes in the docs, how exciting! Should we document that decision somewhere, or just let CI speak for itself?
I feel like the tooling config is the documented decision. |
Works for me! |
3 participants
This PR brings
ruffandpyrightup to newer versions and makes adjustments for those changes.rufffrom 0.11.2 to 0.15.14 and relock to pick uppyright1.1.410.docs/that limits lines to 80 characters to avoid horizontal scrolling in the width-constrained doc view.ops.framework.Framework._event_context'syieldintry/finallyso backend state is restored on exceptions (real cleanup bug surfaced by RUF075, although extremely unlikely to ever happen given where it is).os.path.commonprefixinops.model.Container._build_destpathwithos.path.commonpath(ruff's RUF071 auto-fix). Same shape as the original, just component-based rather than character-by-character, so the partial-component false-match can't happen.@contextmanagerreturn annotations fromIteratortoGeneratorintesting/tests/test_charm_spec_autoload.pyper the new pyright deprecation.This is a big one, but I think even though there is some noise now, it's good as a linter going forward:
--previewformatter now reformats Python code blocks inside markdown — accepted across docs/, README, and the testing/ readmes.There is a catch, though: our docs double-quote strings, and our code single-quotes. ruff doesn't seem to have any clean way of handling this (to be fair, it's a bit odd). One option would be to put a config file in
docs/that inherits from the top level but changes this setting. However, that would miss the README and other similar files, and I assume we don't want those to change either (if we do, then I think it may be the tidier change). I went with running ruff a second time in tox, with a config option overriding the config.