Civet language in Hera code and handlers - #51
Conversation
Adds `language` CompilerOptions field to control handler body language (js, ts, civet). Compiler applies Civet-compatible formatting (indented handlers, single-quoted $EXPECT strings) when language is 'civet'. TS scaffold output is also enabled for civet. Esbuild plugin forwards the option. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a Civet language loader (CJS + ESM hooks) that compiles .hera files with language:'civet' then transpiles the Civet output to JS using @danielx/civet. Exports register/civet, register/civet/cjs, and register/civet/esm from the package. Supports custom civet options via the initialize export on the ESM hook and options.civet on the CJS hook. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a hera.language VS Code setting that controls the handler body language for generated parsers. The LSP server reads the setting and forwards it as a compiler option. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds civet-default (plain civet loader) and civet-custom (with coffeeCompat parseOptions via the initialize hook) loader examples. Includes integration tests via the test-examples script and a README section describing the civet pipeline. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Describes the repo layout, bootstrap problem, build wiring, compiler internals, language pipelines, and testing conventions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@greptileai Could you review this? |
Greptile SummaryThis PR adds a first-class Civet language pipeline to Hera, mirroring the existing TypeScript (tsc) pipeline architecture. Grammars can now declare Key changes:
Confidence Score: 5/5Safe to merge — all remaining findings are P2 style suggestions that do not affect correctness or runtime behavior. No P0 or P1 issues found. The Civet pipeline architecture is clean and mirrors the proven tsc pipeline. The hook registration order, CJS/ESM head selection, and handler-body indentation logic are all correct. The two P2 findings (missing test/register/civet/transpile.civet (missing filename args), lsp/src/server.ts (unhandled promise). Important Files Changed
Sequence DiagramsequenceDiagram
participant GF as .hera file
participant NL as Node ESM/CJS Loader
participant HC as Hera Compiler
participant CT as compileCivetToJs
participant CV as @danielx/civet
participant JS as JavaScript Module
Note over NL: register/civet/index.civet<br/>registers two ESM hooks (LIFO)
GF->>NL: import / require
NL->>HC: heraCompile(src, {language:'civet'})
Note over HC: Uses cjsHead (CJS)<br/>or tsHead (ESM)<br/>+ tsTail + indented handler bodies
HC-->>NL: Civet source string
NL->>CT: compileCivetToJs(civetSrc, url, opts)
CT->>CV: civet.compile({js:true, sync:true})
CV-->>CT: JavaScript string
CT-->>NL: JavaScript string
NL-->>JS: Loaded JS module
Note over GF,JS: esbuild path differs:<br/>civet.compile() without {js:true}<br/>→ TypeScript → esbuild strips types
Reviews (1): Last reviewed commit: "remove extra async" | Re-trigger Greptile |
| getHeraLanguage().then(lang => { | ||
| connection.console.log(`[hera] language: ${lang}`); | ||
| }); |
There was a problem hiding this comment.
getHeraLanguage() is awaited with .then() but has no .catch() handler. Both connection.workspace.getConfiguration(…) and the filesystem reads inside getLanguageFromPackageJson are individually wrapped in try/catch, making a throw unlikely — but connection.workspace.getConfiguration itself can reject if the connection is not yet fully ready on some clients, resulting in an unhandled rejection that surfaces as an uncaught error in the LSP host process.
| getHeraLanguage().then(lang => { | |
| connection.console.log(`[hera] language: ${lang}`); | |
| }); | |
| getHeraLanguage().then(lang => { | |
| connection.console.log(`[hera] language: ${lang}`); | |
| }).catch(err => { | |
| connection.console.error(`[hera] failed to read language setting: ${err}`); | |
| }); |
| "description": "The language used for handlers and code blocks in .hera files." | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Perhaps we should document in this string that this can also be set via package.json setting, but vscode setting overrides?
I'm not actually sure about this priority order. I could imagine making one global setting for vscode in general, but I'd usually trust the package.json of the project I'm in as higher priority...
I also wonder whether it'd make sense to support a meta comment at the top of a Hera file that overrides for that file, in case of multiple .hera's with different languages. I also kind of like the specification being in the Hera file itself. But I also see that the current design doesn't allow multiple languages, and maybe this should be for a future PR.
There was a problem hiding this comment.
I think the LSP side of things needs a whole other PR to work this out including editor integration and sourcemapping (like go to def from inside the handlers)
| if (hasConfigurationCapability) { | ||
| const settings = await connection.workspace.getConfiguration({ section: 'hera' }); | ||
| const lang = (settings as Partial<HeraSettings>).language; | ||
| if (lang === 'javascript' || lang === 'typescript' || lang === 'civet') return lang; |
There was a problem hiding this comment.
A small isLang helper might be nice to avoid the duplication, in case we add more languages.
There was a problem hiding this comment.
Any other languages would be a pretty big rewrite of the compiler since all the mechanisms would need to be implemented in the target language.
Would definitely be cool to get hera for zig eventually though.
|
|
||
| /** | ||
| Quote a string for JS output. Prefer single quotes when the string contains | ||
| double quotes, to avoid backslash-escaped double quotes that Civet can't parse. |
There was a problem hiding this comment.
Is that a bug in Civet? Not sure what we're avoiding here, though I certainly like using single quotes when there's only double quotes in the string, and don't really have a preference when there's both quote types, so this behavior is fine.
There was a problem hiding this comment.
I think this was just the AI sneaking in an extra unnecessary function in confusion, thanks for the double check.
| // bodies and uses single-quoted strings for Civet parser compatibility | ||
| register "../../esm.js", pathToFileURL(__filename), { data: { language: 'civet' } } | ||
| // register the Civet post-processor hook | ||
| register "./esm.js", pathToFileURL(__filename) |
There was a problem hiding this comment.
Wow, hook composition! I take it the registration order equals the application order. I expected to see the transpiler passed into data, but I guess that's how we did TS earlier?
| // createRequire needs a base path for peer dep resolution. | ||
| // process.cwd() is available in both ESM (tests) and CJS (production), | ||
| // and anchors module resolution to the project root where peer deps live. | ||
| _require := createRequire(`${process.cwd()}/package.json`) |
There was a problem hiding this comment.
I'm not sure that process.cwd() is always going to be the project root. It will be when building, but not when running scripts in subdirectories... Oh, but I guess that's still fine, because require will walk up from there. Is there an issue if package.json doesn't exist in that directory, though?
In a perfect world, I imagine we'd pass in context.parentURL from the hook?
There was a problem hiding this comment.
Not too sure about those edge cases, at that point the project configuration is likely far off the beaten path.
| if (options.startRule !== null && options.startRule !== undefined) | ||
| parser = grammar[options.startRule] as Parser<ParserResult<Grammar[K]>> | ||
| else | ||
| parser = Object.values(grammar)[0] as Parser<ParserResult<Grammar[K]>> |
There was a problem hiding this comment.
I think the old style is better, and just add the as casts.
| let civet: any | ||
| /* c8 ignore next 7 */ | ||
| try | ||
| civet = (await import('@danielx/civet')).default ?? await import('@danielx/civet') |
There was a problem hiding this comment.
Surely we don't need this ?? as Civet's exports are consistent.
| catch | ||
| throw new Error [ | ||
| "[@danielx/hera] Civet support requires '@danielx/civet' to be installed." | ||
| "Run: npm install @danielx/civet" |
There was a problem hiding this comment.
| "Run: npm install @danielx/civet" | |
| "Run: npm install -D @danielx/civet" |
It's less clear for the esm plugin, but in an esbuild context I'd say this is a dev dependency.
| "a" -> | ||
| return "ok" as string | ||
| """ | ||
| pluginOptions: { language: 'typescript', loader: 'ts' } |
There was a problem hiding this comment.
| pluginOptions: { language: 'typescript', loader: 'ts' } | |
| pluginOptions: { language: 'javascript', loader: 'ts' } |
Without this change, I don't think we're testing anything.
| @@ -0,0 +1 @@ | |||
| CLAUDE.md No newline at end of file | |||
There was a problem hiding this comment.
I'd rather do the opposite: use AGENTS.md which is standard, and put See @AGENTS.md in CLAUDE.md.
I'm also not really sure we need these files. This paper finds that AGENTS files, especially those autogenerated by LLMs, worsened quality and significantly increased cost. But I leave it to you to decide.
(If we remove them, some of the content might be good for docs though.)
There was a problem hiding this comment.
I think migrating the important info into other docs in the repo that are useful for people and agents is best.
I've also found that AGENTS.md in practice is largely wishful thinking :P
Adds support for Civet inside Hera similar to the existing TS support.
Made some changes to the generated TS and CJS to ensure it is compatible when compiled as Civet including with
coffeeCompat.Working in Civet: DanielXMoore/Civet#1876