ClickHouse / ClickHouse/dbt-clickhouse

Increase number of Ruff rules applied to this project

Open
#694 0 comments 0 reactions 0 assignees View on GitHub
enhancement
Dominant language
Python
Stars
362
Forks
177
Avg merge
2d 10h
Merged PRs (30d)
8

Description

Follow up of https://github.com/ClickHouse/dbt-clickhouse/issues/391

https://github.com/ClickHouse/dbt-clickhouse/pull/679 Introduced Ruff with a good amount of rules to check, but that list can be improved so more things get checked/validated. Adding a list sorted by how impactful would be the changes:

1. **`RUF` (Ruff-specific) — 10 violations.** The highest-value item on the list: RUF012 flags **mutable class defaults** in `dbt/adapters/clickhouse/column.py:13` and `impl.py:68` — class-level dicts shared across all instances, a real shared-state bug class. Also cleans up 6 stale `# noqa` comments (auto-fixable) and 2 unused unpacked variables in tests.

2. **`DTZ` (flake8-datetimez) — 3 violations.** Flags **naive `datetime` calls**, e.g. `datetime.now()` without `tz` in `test_clickhouse_table_ttl.py:63`. Timestamps compared against ClickHouse server time can silently diverge when the CI machine's timezone differs — the classic cause of flaky TTL tests. Tiny fix, real failure mode.

3. **`PGH` (pygrep-hooks) — 6 violations.** All blanket `# noqa` comments without rule codes (in `__init__.py`, `dbclient.py`). A bare `# noqa` suppresses *every* future rule on that line, so bugs introduced there later go unseen. Cheap fix, prevents future blind spots.

4. **`RET` (flake8-return) — 8 violations.** One substantive catch: RET503 at `impl.py:119`, a function that returns a value on some paths but implicitly returns `None` on others — a potential missing-case bug. The remaining 7 are stylistic.

5. **`C90` (mccabe complexity) — 2 violations at the default threshold of 10.** Restores the guardrail our old flake8 config had. At the previous `max-complexity = 18` it passes with **zero changes today** — it purely prevents future complexity growth.

6. **`SIM` (flake8-simplify) — 11 violations.** Readability: mostly SIM118 (`key in dict.keys()` → `key in dict`) in `impl.py` and tests. No bugs, but genuinely cleaner code.

7. **`PT` (flake8-pytest-style) — 13 violations.** Test-suite conventions: parametrize argument types (PT007) and `yield` fixtures with no teardown (PT022). Worthwhile given how test-heavy this repo is, but purely conventional.

8. **`C4` (comprehensions) — 14 violations.** Unnecessary generators passed to `set()` and similar. Micro-performance and style only; auto-fixable with `--unsafe-fixes`.

9. **`UP` (pyupgrade) — 137 violations.** Ranked low not because it's bad but because it's pure modernization (`Optional[str]` → `str | None`, etc.) with the largest diff. All auto-fixable.

10. **`PERF` — 3 violations.** Least impact: try/except inside loops. Would likely need a `# noqa` more than a fix.

items 1–5 (29 violations, mostly small or auto-fixable) buy all the actual bug-catching; 6–10 are style and modernization we can adopt incrementally.

Ones I'd **skip**:
- `S` bandit, 669 hits — almost all false positives from SQL string-building, which is inherent to an adapter.
- `PL` pylint, 139 — mostly magic-number and argument-count pedantry.
- `TRY` 39 — opinionated exception style that conflicts with the adapter's error-wrapping pattern.
- `ARG` 69 — unused arguments that are actually required by dbt's adapter interface signatures.

Contributor guide

Open the contributing guide

Research direction

Review pull request 679 to find the existing Ruff configuration, then run Ruff against the repository and inspect the listed violations in column.py, impl.py, test_clickhouse_table_ttl.py, __init__.py, dbclient.py, and the tests. Enable the selected rule families, resolve the high-impact violations, and confirm the skipped S, PL, TRY, and ARG bands remain disabled.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
tooling
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.