ade2ab95fa
* Remove stale :discouraged-namespace ignores These six ignores suppressed a namespace-level policy that no longer exists in config.edn -- clojure.pprint, clojure.java.io, clj-yaml.core, metabase.util.jvm, and metabase.config.core aren't configured as discouraged namespaces anymore (clojure.pprint is only discouraged at the var level now). Verified clean with a scoped kondo run requesting :discouraged-namespace at :warning over just these files: zero findings. * Give :discouraged-var and :discouraged-namespace their own per-symbol ratchet budgets One shared budget per linter let a new ignore on a previously-clean var land silently as long as the linter's aggregate count had room, and made "introduce a policy for just this one var" impossible. Each configured symbol now gets its own field: :discouraged-var-counts and :discouraged-namespace-counts, siblings of :ignore-counts, keyed to mirror the symbol's own namespace and name rather than the linter that discourages it (metabase.entity-retrieval.core/search-unfiltered -> :entity-retrieval.core/search-unfiltered; an enterprise namespace of the same shape gets an `ee.` prefix so it can't collide with its OSS counterpart). Attribution resolves which symbol an inline ignore covers by running a scoped `clojure -M:kondo` analysis pass over the files that carry one of these two ignores (JSON output, since a generated var can have a name that isn't valid EDN) and matching each ignore to the nearest covered usage of a known symbol -- this is the one thing in the ratchet tooling that needs a live kondo run, so `./bin/mage kondo-ratchets` is no longer pure Babashka. `:comment-exempt` is untouched: occurrences still carry the bare linter name in :linters, only the two new count fields are keyed per symbol. `--seed :discouraged-var`/`:discouraged-namespace` (no specific symbol) seeds every symbol currently configured under it in one shot, unused ones at zero, the same ergonomics as landing a new linter. A stale flat :ignore-counts entry for either linter is self-healing: `fix!` drops it every run and `check` fails if one reappears, since it would otherwise double-count every occurrence the new field already covers. Ported onto the prod/test ratchet split: both ratchet files carry the per-symbol fields, and each file's occurrences are attributed separately. * Attribute discouraged-var ignores from kondo findings, not nearest usage Matching each ignore to the nearest later usage of a configured symbol counted usages kondo never flagged (scoped :config-in-ns entries, :off overrides) and let a stale ignore claim an unrelated usage. Attribution now lints copies of the files with the discouraged ignores disabled, joins each finding to its var or namespace usage at the same row and column, and charges it to the last ignore before it naming the linter. An ignore counts once per file and distinct symbol it covers. The copies rename the ignore key to one of the same length rather than blanking the ignore, so every form reads as before and every row and column stays put. A failed kondo run now throws instead of zeroing every budget. Findings that resolve to no configured symbol fail the check and stop the shrink from writing; ignores that cover no finding only warn. Without an :attribute, a discouraged ignore throws rather than reading as zero. Each ratchet file's occurrences are attributed separately. `--seed :discouraged-var/clojure.core/println` seeds one symbol, a bare `--seed :discouraged-var` seeds every symbol at its actual count, and an empty suffix is rejected. Bulk seeding at zero is gone: the next shrink dropped those zeros anyway. The shrink workflow runs prepare-backend, since the shrink now needs the Clojure CLI. Budgets are re-seeded from the new attribution. Several were undercounted or charged to the wrong symbol before. This squashes the unpushed review-fix commits on top of the per-symbol commit, since #83383 rewrote the same functions each of them changed. * Remove a stale :discouraged-namespace ignore on clj-yaml.core clj-yaml.core is no longer a discouraged namespace, so the ignore on its require in lib-be.metadata.jvm suppresses nothing. * Don't charge findings kondo reports with the ignores in place Attribution charged every discouraged finding on the disabled-ignore copies to the last ignore before it, including findings no ignore suppresses. Unsuppressed usages in dev/ inflated the eval, println and prn budgets, and a new usage of a symbol an earlier ignore already covers went uncounted. Lint the original files in the same kondo run and drop any finding they also report, so only findings an ignore suppresses are attributed. An ignore nested inside another for the same linter still takes the findings under it, leaving the outer one reported as covering nothing. * Parse kondo's JSON output with babashka.json mage already depends on babashka.json, so attribution doesn't need cheshire or the :discouraged-namespace ignore that came with it. That returns the cheshire.core budget to 4. * Attribute prod and test ignores in one kondo run check and fix! attributed the prod and test occurrences in separate calls, and each call started its own JVM and lint pass. :attribute now takes a seq of occurrence groups and returns one attribution per group, so attribute-occurrences! lints every file once and splits the result. kondo-ratchets drops from about 39s to 33s. * Drop budgets for symbols no longer configured instead of refusing Removing a symbol from config.edn left its per-symbol budget naming an unknown key, and both check and the shrink threw on it. The shrink, the tool meant to repair that drift, couldn't. Two PRs could each pass alone and break the shrink on master: one seeding a symbol, the other removing it from the config. check now warns about such a budget, and the shrink drops it at zero like any unused budget, since attribution never charges an unconfigured symbol. --seed still rejects an unconfigured symbol. A bare --seed :discouraged-var no longer warns once per configured symbol with no ignores, and check's ok line counts the per-symbol budgets alongside :ignore-counts. * Share the ratchet setup and config walks between their callers check and fix! repeated the same setup: validate the linters, read the module and test ratchet files, scan, split prod from test, and attribute. They now share read-policies and scan-and-attribute. fix! still resolves and validates --seed before the scan, so a bad seed fails without waiting on kondo. The repository-linter and discouraged-symbol lookups each walked config.edn with an atom to collect nested :linters maps. Both now read them from one linters-maps. The budget-line formatter is shared, and the empty and mergeable field sets derive from ratchet-field-order. Dropped the unused one-argument discouraged-symbols. * Correct the ratchet task's runtime claim and plain up its wording The kondo-ratchets :doc said the check takes seconds; with the kondo attribution pass it takes about half a minute. The CLAUDE.md paragraph and the ratchet-file headers no longer open sentences with code. The unattributed-ignore warning now names a nested ignore for the same linter as a cause: the inner ignore takes the finding, so the outer one reads as covering nothing even though removing it would surface a warning. * Test that a failed kondo run throws rather than reading as no findings If a failed or garbled kondo run read as an empty result, attribution would count zero ignores and the shrink would drop every per-symbol budget. The exit and parse checks move into parse-kondo-output, which tests now cover for a nonzero exit and for output that isn't a map. The attribution tests build matching findings and var usages with one var-hits helper, and the check test's attribution stub reads as what it returns. * Refuse colliding discouraged symbols before an unseeded shrink Dropping the unknown-key validation also dropped the shrink's only call to discouraged-count-keys, which throws when two configured symbols share a ratchet key. An unseeded shrink could then merge both symbols' ignores into one budget. read-policies now reads the configured keys up front, so check and fix! both refuse a collision before scanning. * Reject kondo output without a findings list A run that exits cleanly but prints a map with no :findings, such as {}, parsed as a result with no findings. Attribution would then count zero ignores and the shrink would drop every per-symbol budget. parse-kondo-output now requires :findings to be a list. A missing analysis bucket already fails loudly, since every finding comes back unresolved. * Note where nested ignores leave a budget short Attribution charges a finding to the last ignore before it, so an outer ignore around a nested one for the same linter counts toward no budget. State that next to the rule, where a reader of the code will look for it. * Flag the nested-ignore undercount as a known gap Lead the comment with the gotcha and its consequence, a budget one short that an unattributed warning hides behind, so a reader doesn't mistake the last-ignore rule for a complete one. * Key aliased discouraged vars by the namespace the file requires Kondo resolves a re-exported var to the namespace it learned the var came from, and what it has learned depends on its cache and on which files share the run. lib.core re-exports ->legacy-MBQL from lib.convert, so after an ordinary lint of lib/core.cljc the same ten ignores moved from :lib.core/->legacy-MBQL to :lib.convert/->legacy-MBQL and the check failed. A var used through an alias now takes its namespace from the file's own require for that alias, per language so a .cljc file's clj and cljs requires stay apart, and the attribution lint runs with --cache false. * Split the attribution test by behaviour One deftest each for counting, which findings are charged, and how a finding resolves to a symbol, so a failure names the behaviour that broke. * Say a stale flat discouraged budget is never read, not double-counted The check removes these linters from the occurrences before comparing :ignore-counts, so a leftover flat entry is ignored rather than counted a second time. * Mark the nested-ignore gap as a way past the ratchet, with a TODO A new ignore wrapped around an existing one for the same linter needs no budget, so say that plainly and record the planned fix: lint copies with one ignore disabled each and charge a finding to the copy that reveals it.
137 lines
7.4 KiB
Markdown
137 lines
7.4 KiB
Markdown
# Metabase Development Guide
|
|
|
|
# Skills
|
|
|
|
For detailed guidance on writing and reviewing code and documentation, see the skills in [.claude/skills/](.claude/skills/):
|
|
|
|
## Clojure
|
|
|
|
### clojure-mcp tools
|
|
|
|
- **[clojure-eval](.claude/skills/clojure-eval/SKILL.md)** - Always use this to evaluate Clojure code, **run tests**, and verify edits/compile. Prefer this over shell commands.
|
|
- **[clojure-write](.claude/skills/clojure-write/SKILL.md)** - Clojure/ClojureScript development with REPL-driven workflow and coding conventions
|
|
- **[clojure-review](.claude/skills/clojure-review/SKILL.md)** - Clojure/ClojureScript code review guidelines and style enforcement
|
|
|
|
### clojure-mcp-lite tools
|
|
|
|
- **clj-nrepl-eval** - This is another good mechanism for running Clojure code on an nrepl server.
|
|
|
|
## TypeScript
|
|
|
|
- **[typescript-write](.claude/skills/typescript-write/SKILL.md)** - TypeScript/JavaScript development and best practices
|
|
- **[typescript-review](.claude/skills/typescript-review/SKILL.md)** - TypeScript/JavaScript code review guidelines
|
|
|
|
## Documentation
|
|
|
|
- **[docs-write](.claude/skills/docs-write/SKILL.md)** - Documentation writing with Metabase style guide
|
|
- **[docs-review](.claude/skills/docs-review/SKILL.md)** - Documentation review checklist
|
|
|
|
## Serialization
|
|
|
|
- **[serdes-workflow](.claude/skills/serdes-workflow/SKILL.md)** - Export, validate, and import Metabase content via serdes
|
|
- **[serdes-yaml-edit](.claude/skills/serdes-yaml-edit/SKILL.md)** - Edit exported YAML files with correct portable references
|
|
|
|
## Frontend
|
|
|
|
- **[analytics-events](.claude/skills/analytics-events/SKILL.md)** - Add product analytics events to track user interactions
|
|
|
|
**Important**: When working with frontend code, read [frontend/CLAUDE.md](frontend/CLAUDE.md) for project-specific guidelines on component preferences, styling, testing requirements, and available scripts.
|
|
|
|
## Running Backend Tests
|
|
|
|
If you do not have `clojure-eval` available to you or `clj-nrepl-eval`, do not fall back to `clj -X:dev:test` directly. Instead, use `./bin/test-agent`. It produces clean, plain-text output with no progress bars or ANSI codes.
|
|
|
|
```bash
|
|
./bin/test-agent :only '[metabase.foo-test]' # run a namespace
|
|
./bin/test-agent :only '[metabase.foo-test/some-test]' # run a single test
|
|
./bin/test-agent :only '[metabase.foo-test metabase.bar-test]' # multiple namespaces
|
|
```
|
|
|
|
For module-scoped runs — useful when validating a branch's blast radius — pass `:module` (single) or `:modules` (vector) to scope tests to the module(s) the branch touched. The test runner resolves each module to its test directory through its `:ns-prefix`: `lib.schema` → `test/metabase/lib/schema`, `enterprise/foo` → `enterprise/backend/test/metabase_enterprise/foo` (see `metabase.test-runner/module-folders`).
|
|
|
|
```bash
|
|
./bin/test-agent :module enterprise/workspaces
|
|
./bin/test-agent :modules '[sql-parsing query-processor]'
|
|
# Driver tests: --drivers=LIST adds the driver aliases and sets DRIVERS=LIST in one step.
|
|
./bin/test-agent --drivers=mysql,h2,postgres :module enterprise/workspaces
|
|
```
|
|
|
|
Once again, do not use `clj -X:dev:test` directly — its progress-bar output is hard to parse.
|
|
|
|
## Module Boundaries
|
|
|
|
The linter config at `.clj-kondo/config/modules/config.edn` records each module's `:api`, `:uses`,
|
|
`:model-exports`, and `:model-imports`. `metabase.core.modules-test` fails when it drifts from the source.
|
|
|
|
After **any** backend change that could shift module boundaries, regenerate it:
|
|
|
|
```bash
|
|
./bin/mage fix-modules-config
|
|
```
|
|
|
|
Changes that shift boundaries include: adding/removing/renaming a `src` namespace, adding or dropping a
|
|
cross-module `require` or `:model/X` reference, or creating a new module. When unsure, just run it — it is
|
|
a no-op (exits `unchanged`) when nothing drifted.
|
|
|
|
It piggybacks on a running dev nREPL (~5s) and auto-spawns a JVM if none is running (~15s). It only edits
|
|
the four generated keys; structural changes it can't safely make (a new module needs a human `:team`, or
|
|
modules need reordering) are printed as `WARNING:` lines for you to resolve by hand.
|
|
|
|
Run all repository-level checks, or one named suite:
|
|
|
|
```bash
|
|
./bin/mage project-tests
|
|
./bin/mage project-tests <backend|migrations|modules|ratchets>
|
|
```
|
|
|
|
### Nested modules
|
|
|
|
Module names form a tree: `lib.schema` is a child of `lib`. When OSS module `search` exists,
|
|
`enterprise/search` is its child. Run `./bin/mage modules-tree` to inspect the hierarchy, or add `--html` for an
|
|
interactive explorer with a dependency graph.
|
|
|
|
- Namespace ownership uses the most specific matching prefix. Declaring `lib.schema` assigns
|
|
`metabase.lib.schema.*` to it without moving files. Use `:ns-prefix` when namespaces do not match the
|
|
module name.
|
|
- Every cross-module dependency still requires `:uses`.
|
|
- A child may use an ancestor's internal namespaces. Parents, siblings, and unrelated modules must use the
|
|
target's `:api`.
|
|
- Each `:module-exports` entry widens a nested module's visibility by one ancestor. Export every link to
|
|
make it available everywhere. OSS module `X` exports its `enterprise/X` companion automatically.
|
|
|
|
## Ratchets
|
|
|
|
After changing Clojure suppressions or module-boundary escape hatches, run `./bin/mage kondo-ratchets`.
|
|
Fix the underlying issue when possible; suppressions are a last resort and need a nearby explanation.
|
|
|
|
Inline-ignore budgets (`:ignore-counts`) are split by prod/test so one side's suppressions can never mask
|
|
or look equivalent to the other's: `.clj-kondo/ratchets.edn` covers production code, and
|
|
`.clj-kondo/ratchets-test.edn` covers anything under a `test` directory. `:config-counts`
|
|
(`.clj-kondo/config.edn` suppressions) stays combined in `.clj-kondo/ratchets.edn` regardless -- see that
|
|
file's own header for why. `.clj-kondo/config/modules/ratchets.edn` (module-boundary escape hatches) is a
|
|
separate, unsplit concern.
|
|
|
|
Explain budget increases in the PR. Do not record reductions in feature PRs: post-merge automation opens
|
|
a `Tighten ratchets` PR for them. Use `./bin/mage kondo-ratchets-shrink --seed :linter` only when adding an
|
|
inline-ignore budget. If any ratchet file conflicts, run `./bin/merge-kondo-ratchets`.
|
|
|
|
`.clj-kondo/config/modules/cycle-clusters.edn` names each cyclic cluster (strongly connected component) of the module
|
|
require graph by an anchor module: the cluster holding the anchor carries the name, and its members are computed,
|
|
so a named cluster can grow and shrink without an edit. The big one is `galactic-center`. Names follow a space
|
|
theme; the `name-module-cycle` skill helps pick one.
|
|
|
|
The test `metabase.core.module-cycles-test` fails until every cluster holds exactly one anchor. When you split a
|
|
cluster, name the new half: the failure proposes a name and anchor to add. When two named clusters merge, find
|
|
another way; the failure shows the requires joining them. When an anchor leaves every cycle, anchor its name on
|
|
another member if the cluster survives, or remove the line.
|
|
|
|
Ignores for `:discouraged-var` and `:discouraged-namespace` are budgeted per symbol, not per linter, in both
|
|
files. Use `--seed :discouraged-var/clojure.core/println` to seed one symbol's budget, or a bare
|
|
`--seed :discouraged-var` (or `:discouraged-namespace`) to seed every symbol configured under that linter.
|
|
To learn which symbol an ignore covers, `./bin/mage kondo-ratchets` runs `clojure -M:kondo` over the files
|
|
carrying one, so it needs the Clojure CLI as well as Babashka.
|
|
|
|
## Tool Preferences
|
|
|
|
If `clojure-mcp` tools are available, prefer them over shell-based alternatives for Clojure development.
|