Skip to content

Conversation

@owenpearson
Copy link
Member

@owenpearson owenpearson commented Dec 2, 2025

ruff is faster and better :)

Summary by CodeRabbit

  • Chores

    • Migrated linting toolchain to Ruff, updating CI/CD pipeline and project configuration accordingly.
    • Removed legacy linting configuration files.
  • Refactor

    • Improved type validation robustness by adopting safer type-checking mechanisms throughout the codebase.

✏️ Tip: You can customize this high-level summary in your review settings.

@owenpearson owenpearson requested a review from ttypic December 2, 2025 11:19
@coderabbitai
Copy link

coderabbitai bot commented Dec 2, 2025

Walkthrough

The pull request migrates the linting toolchain from flake8 to ruff across configuration files and GitHub Actions workflow, while also replacing strict type comparisons with isinstance checks for improved Python compatibility and subclass handling in two locations.

Changes

Cohort / File(s) Change Summary
Linting tool migration
.github/workflows/lint.yml, pyproject.toml, setup.cfg
Replaces flake8 with ruff in the lint job, dev dependencies, and configuration. Removes legacy flake8 configuration block from setup.cfg and introduces comprehensive ruff configuration with line length 115, E/W/F/N checks, and per-file ignores for __init__.py.
Type checking refinement
ably/util/crypto.py, test/ably/rest/restchannelpublish_test.py
Changes strict type equality checks from type(key) == str to isinstance(key, str) in two locations, enabling proper subclass handling and more Pythonic type validation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Verify ruff configuration rules (E, W, F, N checks) align with intended linting strictness
  • Confirm isinstance() changes in crypto.py and test file maintain intended type validation behavior
  • Ensure linting rules in ruff are equivalent to or stricter than previous flake8 setup

Poem

🐰 From flake8's nest, we hop away,
To ruff's bright path, a modern day!
Type checks grow gentle with isinstance's care,
No strict comparisons left anywhere,
Configuration cleansed, configuration new,
Migration hops us all the way through! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title clearly and accurately summarizes the main change: migrating the linting toolchain from flake8 to ruff across configuration files and workflow.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ruff

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@ttypic ttypic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
pyproject.toml (1)

47-58: Ruff dev dependency and config align with prior linting setup

Adding ruff to the dev extra and configuring [tool.ruff] / [tool.ruff.lint] to select E, W, F, N (while ignoring N818 and F401 in __init__.py) cleanly mirrors the previous flake8/pep8-naming behavior. The extend-exclude and line-length = 115 choices also look consistent with the existing codebase.

One thing to double-check: ensure the ruff>=0.14.0,<1.0.0 version range matches the versions actually available/used in your CI and local environments so installs don’t fail or unexpectedly jump versions.

Also applies to: 82-97

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 101e2b5 and 215acb6.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (5)
  • .github/workflows/lint.yml (1 hunks)
  • ably/util/crypto.py (1 hunks)
  • pyproject.toml (2 hunks)
  • setup.cfg (0 hunks)
  • test/ably/rest/restchannelpublish_test.py (1 hunks)
💤 Files with no reviewable changes (1)
  • setup.cfg
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: check (3.9)
  • GitHub Check: check (3.13)
  • GitHub Check: check (3.10)
  • GitHub Check: check (3.11)
  • GitHub Check: check (3.12)
  • GitHub Check: check (3.7)
  • GitHub Check: check (3.8)
🔇 Additional comments (3)
test/ably/rest/restchannelpublish_test.py (1)

423-427: Interoperability history check: isinstance use is appropriate

Switching from a strict type(...) equality to isinstance(message.data, type_mapping[expected_type]) is more idiomatic and correctly handles subclasses without changing expected behavior for the current type_mapping. Looks good.

ably/util/crypto.py (1)

144-161: get_default_params: isinstance for string keys is an improvement

Using isinstance(key, str) instead of type(key) == str is more Pythonic and correctly handles str subclasses for base64 decoding. Only behavioral change is that subclassed strings now go through b64decode, which is likely desired but worth keeping in mind if any callers relied on the old behavior.

.github/workflows/lint.yml (1)

34-37: CI lint job correctly migrated to Ruff

Using uv sync --extra dev followed by uv run ruff check cleanly switches the workflow to Ruff and should honor the new pyproject-based configuration without extra flags.

@owenpearson owenpearson merged commit f46c52d into main Dec 2, 2025
10 checks passed
@owenpearson owenpearson deleted the ruff branch December 2, 2025 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants