Skip to content

Expand myron-polish skill with frontmatter and new checklist sections#1283

Open
jwils wants to merge 2 commits into
mainfrom
joshuaw/myron-polish-update
Open

Expand myron-polish skill with frontmatter and new checklist sections#1283
jwils wants to merge 2 commits into
mainfrom
joshuaw/myron-polish-update

Conversation

@jwils

@jwils jwils commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Updates the myron-polish pre-review skill, distilling recurring themes from recent review history into the checklists.

Commit 1 — frontmatter + structural additions

  • Frontmatter (name/description) so the skill surfaces in the skill list and /myron-polish is discoverable.
  • New "Pass 4 — Mechanical sweep" step that greps the diff for themes that recur in new code even after the original comment is resolved: trailing whitespace, no-op churn (lines removed and re-added identically), #: annotations missing the space, _ = casts / respond_to? / alias_method / Struct.new / .select{}.map{}, redundant ElasticGraph:: prefixes inside module ElasticGraph, and hardcoded derived type names.
  • script/quick_build added to the verify sequence before any push, to catch cross-gem fallout that per-gem runs miss.
  • New checklist sections: moves & namespace changes (keep moves reviewable as moves), extension mechanics (one strategy at a time, extended hooks, reuse central mechanisms), collections & small churn (keep Set semantics, targeted tests for incidental bug fixes, drop compatibility-shaped params), and gems & dependencies (runtime deps before dev deps, optional extensions stay optional).
  • Expanded YARD+RBS and examples/generated-artifacts guidance, and a post-review-feedback reload step in the workflow.
  • Fixed a duplicate step number in the workflow (two 6.s → 6./7.).

Commit 2 — items distilled from the last month of reviews

Pulled from Myron's inline comments on PRs #1224, #1231, #1247, #1251, #1252:

Code checklist:

  • Prefer polymorphism over is_a?/instance_of? type checks (adapters/schema types are meant to be polymorphic).
  • Core gems stay ignorant of extensions — extension-specific code belongs in the extension, not a core gem.
  • Fix the root cause across a whole category, not just the reported instance; test at that generality.
  • Comment non-obvious "why" (skip cases, surprising structure); link a tracking issue for known limitations/TODOs.

Tests checklist:

  • A test must not be satisfiable by a degenerate constant return ({}, [], nil, one hardcoded case).
  • Make tests self-contained about details that matter (surface hidden helper setup an assertion depends on).
  • Drive behavior through the public API rather than instantiating internal collaborators directly.
  • Preserve the full set of assertions when relocating a test; don't substitute a thinner version.

- Add skill frontmatter (name/description) so it surfaces in the skill list.
- Add a "Pass 4 — Mechanical sweep" grep step for recurring themes (trailing
  whitespace, no-op churn, `#:` spacing, `_ =` casts, redundant `ElasticGraph::`
  prefixes, hardcoded derived type names).
- Add `script/quick_build` to the verify sequence before any push.
- New checklist sections: moves/namespace changes, extension mechanics,
  collections and small churn, and gems/dependencies.
- Expand YARD+RBS and examples/generated-artifacts guidance, and the
  post-review-feedback reload step.
Distilled from the last month of review comments (PRs #1224, #1231, #1247,
#1251, #1252):

Code:
- Polymorphism over `is_a?`/`instance_of?` type checks.
- Core gems stay ignorant of extensions.
- Fix the root cause across a category, not just the reported instance.
- Comment non-obvious "why"; link a tracking issue for known limitations.

Tests:
- Tests must not be satisfiable by a degenerate constant return.
- Make tests self-contained about details that matter.
- Drive behavior through the public API, not internal collaborators.
- Preserve full coverage when relocating tests.
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.

1 participant