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 1/1/25) -- description

Code Organization

• [ ] Everything ^:private unless used elsewhere
• [ ] No declare when avoidable (public functions near end)
• [ ] Functions under 20 lines when possible
• [ ] No blank 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 linter suppressions use proper format (not #_:clj-kondo/ignore keyword form)

Pattern matching table

Quick scan for common issues:

| Pattern | Issue |

| -------------------------------------------- | ----------------------------------------------------------- |

| calculate-age, get-user | Pure functions should be nouns: age, user |

| update-db, save-model | Missing ! for side effects: update-db!, save-model! |

| snake_case_var | Should use kebab-case |

| Public var without docstring | Add docstring explaining purpose |

| ;; TODO fix this | Missing author/date: ;; TODO (Name 1/1/25) -- description |

| (defn foo ...) in namespace used elsewhere | Should be (defn ^:private foo ...) |

| Function > 20 lines | Consider breaking up into smaller functions |

| /api/dashboards/:id | Use singular: /api/dashboard/:id |

| Query params with snake_case | Use kebab-case for query params |

| New API endpoint without tests | Add tests for the endpoint |

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 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.