Clojure Review

Clojure Code Review Skill

@./../_shared/clojure-style-guide.md @./../_shared/clojure-commands.md

Review guidelines

What to flag:

• Check compliance with the Metabase Clojure style guide (included above)
• If CLOJURE_STYLE_GUIDE.adoc exists in the working directory, also check compliance with the community Clojure style guide
• Flag all style guide violations

What NOT to post:

• Do not post comments congratulating someone for trivial changes or for following style guidelines
• Do not post comments confirming things "look good" or telling them they did something correctly
• Only post comments about style violations or potential issues

Example bad code review comments to avoid:

This TODO comment is properly formatted with author and date - nice work!

Good addition of limit 1 to the query - this makes the test more efficient without changing its behavior.

The kondo ignore comment is appropriately placed here

Test name properly ends with -test as required by the style guide.

Special cases:

• Do not post comments about missing parentheses (these will be caught by the linter)

Quick review checklist

Use this to scan through changes efficiently:

Naming

• Descriptive names (no tbl, zs')
• Pure functions named as nouns describing their return value
• kebab-case for all variables and functions
• Side-effect functions end with !
• No namespace-alias repetition in function names

Documentation

• Public vars in src or enterprise/backend/src have useful docstrings
• Docstrings use Markdown conventions
• References use [[other-var]] not backticks
• TODO comments include author and date: ;; TODO (Name 2025-01-01) -- description

Docstring content — check every new or rewritten docstring against the anti-pattern table in the style guide above. Read the diff's docstrings on their own, separately from the code: prose is where review attention slides off, and a long docstring reads as diligence rather than as the liability it usually is.

Code Organization

• Everything ^:private unless used elsewhere
• No declare when avoidable (public functions near end)
• Functions under 20 lines when possible
• No blank, non-comment lines within definition forms (except pairwise constructs in let/cond)
• Lines ≤ 120 characters

Tests

• Separate deftest forms for distinct test cases
• Pure tests marked ^:parallel
• Test names end in -test or -test-<number>

Modules

• Correct module patterns (OSS: metabase.<module>.*, EE: metabase-enterprise.<module>.*)
• API endpoints in <module>.api namespaces
• Public API in <module>.core with Potemkin
• No cheating module linters with :clj-kondo/ignore [:metabase/modules]

REST API

• Response schemas present (:- <schema>)
• Query params use kebab-case, bodies use snake_case
• Routes use singular nouns (e.g., /api/dashboard/:id)
• GET has no side effects (except analytics)
• Malli schemas detailed and complete
• All new endpoints have tests

MBQL

• No raw MBQL manipulation outside lib, lib-be, or query-processor modules
• Uses Lib and MBQL 5, not legacy MBQL

Database

• Model and table names are singular nouns
• Uses t2/select-one-fn instead of selecting full rows for one column
• Logic in Toucan methods, not helper functions

Drivers

• New multimethods documented in docs/developers-guide/driver-changelog.md
• Passes driver argument to other driver methods (no hardcoded driver names)
• Minimal logic in read-column-thunk

Miscellaneous

• Example data is bird-themed when possible
• Kondo suppressions use a reader-discard map, not #_:clj-kondo/ignore, with :clj-kondo/ignore as the first key
• Each new suppression is approved and has a ;; comment explaining why the warning cannot be fixed
• Any ratchet budget increase is explained in the PR
• PRs that remove ignores leave ratchet budgets unchanged; the shrink workflow records reductions on master

Pattern matching table

Quick scan for common issues:

Feedback format examples

For style violations:

This pure function should be named as a noun describing its return value. Consider user instead of get-user.

For missing documentation:

This public var needs a docstring explaining its purpose, inputs, and outputs.

For docstrings documenting code they don't own:

This describes how lib decides binning strategies internally. Nothing here fails when that changes, so it will go stale silently — drop it and let available-binning-strategies answer for its own behavior.

For docstring content that belongs in the body:

This paragraph explains why the branches are ordered this way, which is what someone editing the cond needs — but a caller can't act on it. Move it to an inline comment above the cond.

For organization issues:

This function is only used in this namespace, so it should be marked ^:private.

For API conventions:

Query parameters should use kebab-case. Change user_id to user-id.