paulk-asert commented on PR #2927: URL: https://github.com/apache/groovy/pull/2927#issuecomment-5657338665
Initial AI read: > **Verdict: sound, merge after two fixes plus one API decision.** > > The PR delivers exactly the four seams GROOVY-12403 asks for, without changing the default `groovysh` path, and quietly fixes a real bug on the way (`GroovyEngine.Inspector` was building its shell with `CompilerConfiguration.DEFAULT`). > > Verified locally on a worktree of `refs/pull/2927/head`: compiles, `:groovy-groovysh:lintGate` passes, module suite 195 passed / 2 skipped, clean trial merge with today's master. > > **Two things to fix in this PR:** > > 1. **Docs don't render.** The three new `include::` directives in `groovysh.adoc` (1465, 1476, 1504) lack the `[source,groovy] ---- … ----` wrapper. I built the HTML — all three examples come out as prose paragraphs, split at the blank line. > > 2. **`commandInfo(String)` ignores its argument** and returns the whole POSIX name list. Pre-existing, but the PR makes it public API *and* adds `commandInfoReturnsPosixCommandNames` to pin it. Captured live from a real `GroovySystemRegistry`: > ``` > /clear date > /date date > /grep date > /ls date ← all 13 console commands > /help command help ← other registries are fine > ``` > > **One team decision:** the new `@Incubating` surface publishes three JLine types that JLine marks `@Deprecated(since="4.0")` — `CommandRegistry`, `ConsoleEngine`, `SystemRegistry` (confirmed by `javap` on the pinned 4.4.3 jar). groovysh already depends on them internally; the new part is advertising them as a *supported* extension point. My recommendation is the cheap option: keep `@Incubating` and state the coupling in the adoc. > > Also flagged: an undocumented class-loader *adoption* rule in the 3-arg `GroovyEngine` ctor (shared `purgeClassCache`), extra registries that can only append and never override, `@Incubating` on only half the new surface, a non-daemon busy-spin thread in the `ThreadInterrupt` test that would survive a failed assertion into 39 more test classes, and nine smaller items. Full detail and grounding in §3. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
