aiskillstore/clojure-review

Clojure Code Review Skill

Metabase Clojure Style Guide

This guide covers Clojure and ClojureScript coding conventions for Metabase. See also: CLOJURE_STYLE_GUIDE.adoc for the Community Clojure Style Guide.

Naming Conventions

General Naming:

• Acceptable abbreviations: acc, i, pred, coll, n, s, k, f
• Use kebab-case for all variables, functions, and constants

Function Naming:

• Pure functions should be nouns describing the value they return (e.g., age not calculate-age or get-age)
• Functions with side effects must end with !
• Don't repeat namespace alias in function names

Destructuring:

• Map destructuring should use kebab-case local bindings even if the map uses snake_case keys

Documentation Standards

Docstrings:

• Every public var in src or enterprise/backend/src must have docstring
• Format using Markdown conventions
• Reference other vars with [[other-var]] not backticks

Comments:

• TODO format: ;; TODO (Name M/D/YY) -- description

Code Organization

Visibility:

• Make everything ^:private unless it is used elsewhere
• Try to organize namespaces to avoid declare (put public functions near the end)

Size and Structure:

• Break up functions > 20 lines
• Lines ≤ 120 characters
• No blank lines within definition forms (except pairwise let/cond)

Style Conventions

Keywords and Metadata:

• Prefer namespaced keywords for internal use: :query-type/normal not :normal
• Tag variables with :arglists metadata if they're functions but wouldn't otherwise have it

Tests

Organization:

• Break large tests into separate deftest forms for logically separate test cases
• Test names should end in -test or -test-<number>

Performance:

• Mark pure function tests ^:parallel

Modules

OSS Modules:

• Follow metabase.<module>.* pattern
• Source in src/metabase/<module>/

Enterprise Modules:

• Follow metabase-enterprise.<module>.* pattern
• Source in enterprise/backend/src/metabase_enterprise/<module>/

Module Structure:

• REST API endpoints go in <module>.api or <module>.api.* namespaces
• Put module public API in <module>.core using Potemkin imports
• Put Toucan models in <module>.models.*
• Put settings in <module>.settings
• Put schemas in <module>.schema

Module Linters:

• Do not cheat module linters with :clj-kondo/ignore [:metabase/modules]

REST API Endpoints

Required Elements:

• All new endpoints must have response schemas (:- <schema> after route string)
• All endpoints need Malli schemas for parameters (detailed and complete)
• All new REST API endpoints MUST HAVE TESTS

Naming Conventions:

• Query parameters use kebab-case
• Request bodies use snake_case
• Routes use singular nouns (e.g., /api/dashboard/:id)

Behavior:

• GET endpoints should not have side effects (except analytics)
• defendpoint forms should be small wrappers around Toucan model code

MBQL (Metabase Query Language)

Restrictions:

• No raw MBQL introspection outside of lib, lib-be, or query-processor modules
• Use Lib and MBQL 5 in new source code; avoid legacy MBQL

Database and Models

Naming:

• Model names and table names should be singular nouns
• Application database uses snake_case identifiers

Best Practices:

• Use t2/select-one-fn instead of fetching entire rows for one column
• Put correct behavior in Toucan methods, not separate helper functions

Drivers

Documentation:

• New driver multimethods must be mentioned in docs/developers-guide/driver-changelog.md

Implementation:

• Driver implementations should pass driver argument to other driver multimethods
• Don't hardcode driver names in implementations
• Minimize logic inside read-column-thunk in JDBC-based drivers

Miscellaneous

Examples:

• Example data should be bird-themed if possible

Linter Suppressions:

• Use proper format for kondo suppressions
• No #_:clj-kondo/ignore (keyword form)

Configurable Options:

• Don't define configurable options that can only be set with environment variables
• Use :internal defsetting instead

Linting and Formatting

• Lint PR: ./bin/mage kondo-updated master (or whatever target branch)
  • Call the command one time at the beginning, record the results, then work through the problems one at a time.
  • If the solution is obvious, then please apply the fix. Otherwise skip it.
  • If you fix all the issues (and verify by rerunning the kondo-updated command):
    • commit the change with a succinct and descriptive commit message
• Lint File: ./bin/mage kondo <file or files>
  • Use the linter as a way to know that you are adhering to conventions in place in the codebase
• Lint Changes: ./bin/mage kondo-updated HEAD
• Format: ./bin/mage cljfmt-files [path]

Testing

• Run a test: ./bin/mage run-tests namespace/test-name
• Run all tests in a namespace: ./bin/mage run-tests namespace
• Run all tests for a module: ./bin/mage run-tests test/metabase/notification Because the module lives in that directory.

Note: the ./bin/mage run-tests command accepts multiple args, so you can pass ./bin/mage run-tests namespace/test-name namespace/other-test namespace/third-test to run 3 tests, or ./bin/mage run-tests test/metabase/module1 test/metabase/module2 to run 2 modules.

Code Readability

• Check Code Readability: ./bin/mage -check-readable <file> [line-number]
  • Run after every change to Clojure code
  • Check specific line first, then entire file if readable

REPL Usage

Note: If you have clojure-mcp tools available (check for tools like clojure_eval), always prefer those over ./bin/mage -repl. The MCP tools provide better integration, richer feedback, and avoid shell escaping issues. Only use ./bin/mage -repl as a fallback when clojure-mcp is not available.

• Evaluating Clojure Code: ./bin/mage -repl '<code>'
  • See "Sending Code to the REPL" section for more details

Sending Code to the REPL

• Send code to the metabase process REPL using: ./bin/mage -repl '(+ 1 1)' where (+ 1 1) is your Clojure code.
  • See ./bin/mage -repl -h for more details.
  • If the Metabase backend is not running, you'll see an error message with instructions on how to start it.

Working with Files and Namespaces

1. Load a file and call functions with fully qualified names:

To call your.namespace/your-function on arg1 and arg2:

./bin/mage -repl --namespace your.namespace '(your-function arg1 arg2)'

DO NOT use "require", "load-file" etc in the code string argument.

Understanding the Response

The ./bin/mage -repl command returns three separate, independent outputs:

• value: The return value of the last expression (best for data structures)
• stdout: Any printed output from println etc. (best for messages)
• stderr: Any error messages (best for warnings and errors)

Example call:

./bin/mage -repl '(println "Hello, world!") '\''({0 1, 1 3, 2 0, 3 2} {0 2, 1 0, 2 3, 3 1})'

Example response:

ns: user
session: 32a35206-871c-4553-9bc9-f49491173d1c
value:  ({0 1, 1 3, 2 0, 3 2} {0 2, 1 0, 2 3, 3 1})
stdout:  Hello, world!
stderr:

For effective REPL usage:

• Return data structures as function return values
• Use println for human-readable messages
• Print errors to stderr

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.