Skip to content

feat(rules): add Clojure and ClojureScript review rules - #965

Open
chethanuk wants to merge 1 commit into
alibaba:mainfrom
chethanuk:wf/clojure-language-support
Open

feat(rules): add Clojure and ClojureScript review rules#965
chethanuk wants to merge 1 commit into
alibaba:mainfrom
chethanuk:wf/clojure-language-support

Conversation

@chethanuk

Copy link
Copy Markdown
Contributor

Description

.clj, .cljs, .cljc and .edn files were never reviewed. None of the four extensions is in supported_file_types.json, so IsAllowedExt drops them at the filter gate before any rule can run, which is why a rule doc on its own would never have been reachable.

This adds all four and registers one rule doc in path_rule_map: **/*.{clj,cljs,cljc,edn}clojure.md. Data only, no Go source changes.

One doc for all four extensions. ClojureScript is the same language on a different host, and .cljc compiles to both, so a Clojure/ClojureScript split has no correct answer for .cljc: path_rule_map is first-match-wins, so whichever of the two patterns is listed first takes it and the other host's rules never load. Multi-extension docs already cover that case, haskell.md for .hs/.lhs and nim.md for .nim/.nims/.nimble. EDN is data rather than code, so it gets its own section: dependency coordinates pinned to a branch instead of an exact :git/sha, aliases whose :main-opts execute code, and read-string where clojure.edn/read-string belongs.

Exclude patterns are scoped by extension rather than by bare directory, since IsExcludedPath applies every pattern to every path with no language dispatch. Test files are **/test/**/*.{clj,cljs,cljc} and **/*_test.{clj,cljs,cljc}.

Compiled output needs **/resources/public/js/compiled/**/*.{js,cljs,cljc,edn}. That directory is the lein-cljsbuild and lein-figwheel :output-to/:output-dir convention, and the tree it writes holds the Closure bundle, verbatim copies of every ClojureScript source it compiled, and per-namespace *.cljs.cache.edn analysis caches. All four of those extensions are allowlisted, .js already before this PR, so without the pattern a diff that touches that tree gets cljs/core.cljs reviewed as if someone had written it. resources/public/js/vendor/analytics.js is a negative row: hand-written JS one directory over stays reviewable.

**/.cpcache/** and **/.shadow-cljs/** are the only bare directories. Both names are owned by a tool and never typed by hand, and both trees hold .edn files (launchpad writes .cpcache/launchpad.edn, shadow-cljs writes builds/*/*/bundle-info.edn), so both can actually fire now that .edn is allowlisted.

Nothing is added for target/. providerDirIgnoreDirs in internal/diff/git.go already drops it at the diff level, before the allowlist is reached. out/ is left alone because real Clojure .gitignores root-anchor it as /out/, which this matcher cannot express, and **/out/** would drop any directory named out repo-wide.

Limitation

Clojure diffs that used to be skipped are now reviewed. Anyone who does not want that opts out with user exclude globs, which are checked before the allowlist.

Type of Change

  • New feature (non-breaking change that adds functionality)

How Has This Been Tested?

  • make test passes locally
  • Manual testing (describe below)

Rows appended to the existing table tests:

  • TestIsAllowedExt.clj/.CLJ, .cljs/.CLJS, .cljc/.CLJC and .edn/.EDN.
  • TestIsExcludedPath — both test-file patterns at the repo root and nested; the compiled bundle, a copied cljs/core.cljs, a *.cljs.cache.edn and a subproject-prefixed output path; .cpcache and .shadow-cljs; and three negatives, src/myapp/core.clj, src/myapp/test_helpers.clj and the hand-written JS above.
  • TestResolve_DefaultRules.clj, .cljs and .cljc paths plus deps.edn resolve by heading anchors unique to clojure.md.

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (if applicable)
  • I have signed the CLA

The rule table in pages/src/content/docs/{en,ja,ru,zh}/review-rules.md lists the new pattern.

Related Issues

Part of #470.

@github-actions

Copy link
Copy Markdown
Contributor

OpenCodeReview: Review complete: 0 finding(s) across 3 selected item(s).

@chethanuk
chethanuk force-pushed the wf/clojure-language-support branch from 02bb357 to 2aa9ccb Compare August 17, 2026 06:18
@chethanuk

Copy link
Copy Markdown
Contributor Author

@lizhengfeng101 Please review and merge this. Future main updates might cause more conflicts. Thanks :)

@chethanuk
chethanuk force-pushed the wf/clojure-language-support branch from 2aa9ccb to 71931ce Compare August 17, 2026 07:37
Allowlist .clj/.cljs/.cljc/.edn and route them to a single clojure.md
rule doc. ClojureScript is the same language on another host and .cljc
compiles to both, so a split doc has no correct answer for .cljc under
the first-match-wins router. EDN is included because deps.edn and
friends carry real reviewable risk (unpinned git coords, aliases that
run code, read-string vs clojure.edn/read-string); it gets its own
section since it is data, not code.

Exclude patterns are scoped by extension, never bare directories:
IsExcludedPath applies every pattern to every path with no language
dispatch. The compiled-output pattern covers .js/.cljs/.cljc/.edn under
resources/public/js/compiled/, which is what the lein-cljsbuild and
lein-figwheel :output-to/:output-dir convention emits: the Closure
bundle, verbatim copies of the ClojureScript sources it compiled, and
per-namespace *.cljs.cache.edn analysis caches. All four extensions are
now allowlisted, so without it a diff that touches that tree reviews
cljs/core.cljs as if someone had written it.

.cpcache and .shadow-cljs stay because both hold .edn (launchpad writes
.cpcache/launchpad.edn, shadow-cljs writes builds/*/bundle-info.edn) and
both are tool-owned names nobody writes by hand. target/ is already
dropped upstream by providerDirIgnoreDirs, and out/ is skipped because
real Clojure gitignores root-anchor it, which this matcher cannot
express. No pattern is added for .lein-*, .nrepl-port or .cpcache/*.cp:
none of those extensions is allowlisted, so IsAllowedExt rejects them
before IsExcludedPath is consulted.
@chethanuk
chethanuk force-pushed the wf/clojure-language-support branch from 71931ce to 3b4b7d6 Compare August 17, 2026 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants