Imported from henrygilbert22/ai-agent-tools (skills/review-like-henry/SKILL.md). Install upstream with npx skills add henrygilbert22/ai-agent-tools --skill review-like-henry. Copyright stays with the author.

Review & Code Like Henry

This skill encodes how Henry Gilbert reviews PRs and writes code, distilled from his actual history in this repo (2022–2026): 4,309 inline review comments, 532 review summaries, 549 discussion comments across 582+ PRs he reviewed, and 1,140 PRs he authored. Quotes below are verbatim from his reviews, tagged with the PR they came from, so the voice stays authentic.

Use it two ways:

• As a reviewer: walk the Review checklist and adopt the voice.
• As an author: follow Code conventions so the diff would survive his review.

Core philosophy (the throughlines)

Everything below reduces to a handful of beliefs he applies relentlessly:

1. Explicit over implicit. Strong types over str/dict/Any, named constants over magic values, raise over silent defaults, tuple-unpacking over positional indexing, required params over silent defaults. If intent can be encoded in the type system or a name, encode it.
2. Correctness under failure. Validate inputs (especially external/vendor data), forbid impossible states, and fail loud — never swallow, downgrade, or silently skip an error.
3. Simplicity / YAGNI. Interrogate whether each class, file, field, param, and abstraction needs to exist. "optimizing for ultimate extensibility is what made the old system so obtuse" (PR #44980). He is anti-over-engineering but also anti-dogma — no conventions for their own sake.
4. Testability is a design property. "good code is easily testable code" (PR #35180). Inject dependencies and time sources; avoid globals; keep IO out of pure logic.
5. Clean layering. Domain objects vs. DAOs/IO; pure logic shouldn't load its own data; production code must never depend on research/experimental code.
6. Readability for the next maintainer. "Calling two functions instead of one is always preferable if it means people can understand it" (PR #7-series). Self-explanatory code over comments.
7. Runtime truth over repository theater. A filename, comment, BUILD target, or passing test does not prove code is live. Start from real launchers, deployed services, scheduled jobs, importers, and persisted contracts; follow the call graph to the implementation.
8. Delete structural debt instead of organizing it. Dead code, one-off tooling, compatibility facades, generated evidence, and agent process documents do not become production quality when moved into tidy subdirectories. Retain only code with a real owner and recurring runtime or operator role.

He is pragmatic, not absolutist: he defers out-of-scope cleanup to scoped follow-up PRs, accepts justified stop-gaps, marks taste as taste (nit:), and approves to unblock while naming must-do follow-ups.

How Henry comments (voice & process)

Match this voice when leaving review comments.

• Lead with a Socratic question, not a directive. His single most common move is "Why are we…?", "Why not…?", "What if…?", "When would this be used?", "Do we need this?". The question carries the critique while leaving the author room to justify. Soften with trailing "no?" / "correct?" to invite agreement.
• Scope severity explicitly. Prefix non-blocking style/taste with nit:. Call out hard blocks plainly: "Overall mostly nit comments; however, I do think we should hard block on the following…" (PR #22786). Distinguish principle from preference: "this is a non-blocking comment and is likely closer to preference than anything" (PR #27809).
• Be terse. Many comments are 1–6 words: "Enum?", "kill", "type?", "constants!!!", "classmethod", "log don't print", "validate", "testing?".
• Offer the fix, not just the objection. Supply a concrete alternative snippet or "Alternatively: …" rather than only flagging the problem.
• Praise warmly and briefly when earned — "LOVE IT", "Noice", "beautiful", "this is sexy - love it", "Always love to see 28k lines of code deleted" (PR #36405), "Kudos for your general test writing and documentation! You're very easy to build things with" (PR #26837).
• Close loops traceably. Reference resolving commit SHAs ("Resolved in: a400ce6"), tickets (RES-/RESH-), and tag owners for cross-cutting calls (@andrew-arta do we want to do that in this PR?).
• Blunt but collaborative. When a design is wrong he says so ("This seems crazy complex for what it's doing.", "Have a little self respect", "this is a hack - we should fail on this"), and he holds himself to the same bar (often harshest on his own PRs). Dry humor is fine.
• Push for coherent, reviewable change. Split unrelated behavior/proto changes and rebase out incidental edits. Do not force an atomic package move into dozens of stacked PRs when that creates compatibility layers and merge tax; one behavior-preserving structural PR can be the cleaner unit.

Approve / block calibration

He comments far more than he formally blocks (historically ~660 approvals vs. ~11 formal "changes requested"). Default to COMMENTED with clear nits; reserve hard blocks for real architectural problems, correctness/safety issues, or missing tests on critical logic.

• High-risk override: for AI-heavy diffs, mega-refactors, moves/renames, or “behavior preserved” claims, inability to prove scope, reachability, ownership, or operational parity is itself a hard block. Do not stamp to unblock and defer debt introduced by this PR to a follow-up.
• Quick approve: clean functional decomposition, strong tests, code deletion/simplification, correct use of existing abstractions (DomainProtocol, generics), good validation/docs.
• Approve-with-conditions is common: "lgtm with a few nit comments", "LGTM but let's add testing", "lgtm (assuming resolution of existing comments)", "stamped to unblock" + named follow-up.

Review checklist

Walk these in roughly this priority order. Each item = what to look for + how he phrases it.

0. AI de-slop / structural integrity (hard-block by default)

Treat AI output as an untrusted patch, not an authored design. It can be locally correct, typed, and tested while still making the codebase materially worse. Review the system before polishing its lines. For large, structural, agent-authored, or file-heavy PRs, complete this section before the normal checklist.

Prove reachability and ownership

• Start from real entrypoints: deployed binaries/services, cron or workflow drivers, shell/CLI launchers, API handlers, poll loops, and importers. Follow imports and calls inward. Do not infer liveness from names, comments, tests, exports, or BUILD membership.
• Classify every added, deleted, moved, or substantially changed file as one of: live service, shared library, recurring operator workflow, one-off/migration, generated artifact/evidence, or dead. Every retained file needs a named runtime/operator owner and a concrete caller. Every deletion needs an ownership/retention decision. “Might be useful later” means delete.
• Search outside the edited folder for consumers, facade re-exports, string-built labels/paths, deploy manifests, tests that invoke scripts, and sibling services. A local review of the obvious package is insufficient.
• Treat tests for unreachable code as dead code too. A test proves behavior if called; it does not prove the product or service calls it.

Delete debris aggressively — without stealing ownership

• Default-delete agent handoffs, progress logs, implementation plans/specs, adversarial-audit writeups, screenshots, generated result dumps, caches, baselines, and committed run evidence from production packages. Keep durable operational truth in at most one central README/ARCHITECTURE document per real package unless the owner explicitly requests another document.
• Default-delete recent one-off inspection, migration, scale, backfill, and remediation tools once their job is complete. Retain a tool only when there is a recurring operator use case, a stable entrypoint, safety boundaries, an owner, and tests. Put retained operational tools under the owning service's ops/, not in a “generic” library hallway.
• Do not delete or rewrite another owner's ambiguous code or documents by taste. Prove they are dead or ask the owner. If the owner wants historical/design material retained, move it to an explicit owner-approved location while preserving its substance. De-slop is not license for drive-by scope.
• Never commit an agent's own progress/handoff document merely to explain a PR. Put the plan in the PR or conversation; the repository is the product, not the agent's scratchpad.

Make package boundaries tell the runtime truth

• “Generic” is a strong claim. Grep for partner names, realm defaults, contract IDs, product names, target-specific prompts/metrics, and hardcoded report vocabulary. Move them under the owning target or partner package. Generic layers contain contracts and mechanisms, not one partner's policy.
• Organize by actual service responsibility: generation, ingest, evaluation, reporting, target adapters, polling, daily jobs, artifact publication, operator workflows. Avoid history-shaped hallways such as eval/adapters/utils/common when they mix independent runtimes.
• Reject parallel import, grading, reporting, rendering, or artifact paths unless the PR names the canonical owner and an active migration. Keep computation, rendering, and transport separate, with one explicit payload boundary; do not let sibling packages import each other's internal implementations.
• For any vaguely named “artifact” layer, require an exact chain: producer -> schema/version -> object or table location -> consumer -> recovery/rebuild path. If nobody can state that chain, block the layer. Do not delete a live artifact protocol because a retired optional feature used the word “artifact.”
• Reject package barrels, wildcard exports, pass-through facades, duplicate wrappers, and compatibility aliases created only to make an atomic move easier. Update real importers, BUILD labels, launchers, and string paths. A compatibility layer needs a demonstrated external consumer, owner, and removal plan.
• Distinguish persisted identity from source layout. A historical fingerprint/key may intentionally keep an old literal; document and test that exception instead of “cleaning” persisted contracts.

Enforce a complexity budget

• Treat a production file approaching 1,000 LOC as a decomposition failure. Hard-block new 1k-line files. A PR touching an existing god file must name ownership-based split targets and should not make the file larger without an exceptional reason. Scrutinize any ~300+ LOC addition to one file.
• The ceiling is a tripwire, not proof of quality. Several 700-line files separated by decorative headings can still be one god system. Review cohesion, dependency direction, and whether each file has one independently explainable responsibility.
• A new class/layer must enforce an invariant, own a lifecycle, or remove meaningful duplication across real consumers. Block one-method classes, renamed-dict dataclasses, forwarding wrappers, speculative plugin systems, and abstractions with one caller.
• Every new flag, environment variable, CLI option, BUILD target, and extension seam needs a current caller and a test through that caller. “Leave it for future re-enable” is dead API unless explicitly owner-approved.
• Reject ancillary “library hygiene,” launcher APIs, key renames, and nearby cleanups that are not needed for the stated outcome. A structural cleanup is not permission to freelance behavior changes.
• Prefer deletion over a quarantine directory. Use experimental/<owner>/ only for an active experiment with a named owner and expiry, never as a landfill for uncertain code.

Freeze behavior during structural cleanup

• Write the behavior boundary before moving code: prompts/rubrics, criterion order, thresholds, normalization vocabulary, report keys, artifact object layout, durability ordering, API semantics, and product decisions. Structural cleanup must preserve these unless the PR explicitly changes and validates them.
• Move code into real owners atomically; update imports, runfiles, BUILD deps, shell paths, workflows, deploy configuration, and consumers in the same change. Do not leave a facade because the migration was split poorly.
• Compare implementations mechanically where behavior must stay identical. Review renamed code for the small non-rename delta; do not trust “move only” claims.

Demand runtime evidence, not just CI

• Static/unit checks are necessary but insufficient for launchers and services. Run the documented local command on a representative multi-item workload; launch affected APIs/UIs/workers; hit health and core endpoints; confirm logs show traffic reaching the intended local process; inspect produced artifacts.
• Validate both supported execution paths at clean boundaries (for example Python mode and Bazel mode) without adding mode-specific behavior. Check flags and environment variables forward end-to-end.
• Verify producers and consumers together: generate/import -> grade -> report -> publish/load. Count inputs, outputs, grader calls, terminal states, manifest objects, or other contract-specific evidence.
• Inspect transcripts/output content, not only exit codes. Kill started processes and prove leased ports and temporary state are clean afterward.
• Call out CI-invisible tests, generated-module dependencies, shared-state order dependence, and backfill code imported by production services. Passing the default test target is not enough when the real entrypoint takes another path.

Recognize common AI smells

Hard-question or delete code exhibiting several of these together:

• narrating comments, decorative section rulers, verbose docstrings, or prose that restates the code;
• utils.py, common.py, manager, provider, adapter, or framework layers without one crisp owner;
• inline imports, broad Any/object, getattr fallbacks, silent defaults, catch-and-continue, or copied configuration plumbing used to make incompatible layers appear generic;
• duplicate models/constants, facade re-exports, wrapper functions with one caller, and tests that only assert shapes, mocks, or implementation details;
• TODO/future/re-enable flags with no caller; one-off scripts beside live services; generated JSON/Markdown evidence; filenames containing handoff/progress/plan/audit/change-log without explicit owner justification;
• giant “comprehensive” modules or tests that combine unrelated services because generation was easier than finding the correct ownership boundary.

Scan the added lines mechanically for these tells before approval. A retained section banner, compatibility wrapper, generic helper, or long explanatory comment needs a concrete invariant or runtime contract—not merely a plausible story.

For a large structural review, return concrete evidence rather than vibes: current runtime/service map, confident kill list plus explicit borderline-owner questions, end-state ownership tree with named god-file splits, behavior-freeze list, importer/BUILD/entrypoint migration risks, and exact runtime validation gates. Hard-block unreachable code, partner policy in generic packages, new god files, agent process debris, facades for atomic moves, artifact layers without a producer/consumer chain, and flags without callers.

1. Typing strictness (very common)

• Reject str/dict/Any/object where an Enum, Literal, generic, TypeVar, NamedTuple, or domain type belongs. "Why is shadow a string? That feels very brittle" (PR #24963); "str types are a bad vibe. Make an explicit enum if possible" (PR #45792).
• Parameterize generics: DomainProtocol[measurement_pb2.LargeTraderSecurityMeasurement], not bare DomainProtocol (PR #53462).
• # type: ignore only for untyped third-party packages — never to silence your own code. A # type: ignore that hides a circular import "likely indicates a fundamental misconfiguration" (PR #15323).
• Make optionality explicit; flag X | None where existence should be enforced, and None-as-sentinel ambiguity ("is default value of 0 valid? … Consider defaulting to None instead", PR #27809). But also trim gratuitous optionality (don't check for None the type already forbids, PR #2109).
• Use domain ID types (AssetId, PortfolioId) and internal value types (ArboDecimal, ArboTimestamp) over raw str/float/datetime. Union types like as_of: ArboTimestamp | date are a "Code smell" (PR #52439). Rationale he states: "Because python isn't type safe … No error just all mismatches" (PR #53406).

2. Input validation & impossible states (very common)

• Never assume non-empty / well-formed / sorted data, especially from external vendors: "How do you know it's sorted? We should never assume external vendors will do anything correctly" (PR #29580); "What if there are no keys? This will fail" (PR #45792).
• Validate at construction (__post_init__ / from_proto / _validate_*) so invalid states are unrepresentable. "Is target_protection_level == 0 a valid state if the risk factor is specified? This should not be supported correct?" (PR #18156).
• Defaults must be valid and explicit: "Don't set a default as it's not valid. This would mean no one can use it" (PR #37998); "Bad pattern, everything should be explicitly matched no defaults" (PR #55510).

3. Error handling — fail loud (very common)

• raise in production; assert is for tests/invariants only. "assert is for testing, raise exceptions" (PR #26666); "Consider using raise vs assert so we can add messages and be explicit about what's failing and why" (PR #13555).
• Don't swallow/downgrade: no bare warn-and-continue, no catch-then-reraise, no silent fallthrough. "this is a hack - we should fail on this" (PR #54643); "Why? Errors aren't expected here so if one pops up we should fail everything" (PR #54717); "i'd rather allow the index error here than silently handle it" (PR #54643).
• Raise with information-rich messages (use the self-documenting f"{var=}" form); prefer specific exception subclasses over bare Exception; add an else: raise to exhaustive if/elif chains.

4. Constants over magic values (very common)

• Hoist hardcoded numbers/strings/lists into named module/class constants (UPPER_SNAKE), or make them parameters/flags. "4 should be a parameter, not hard coded" (PR #3746); "Why do you hate constants??" (PR #44982); "constants!!!" (PR #54760).

5. Simplicity / YAGNI / dead code (very common)

• Interrogate necessity of every new field/param/class/file/abstraction: "this could just be a NamedTuple … definitely not deserving of it's own file" (PR #26420); "Why are we making a class for this? … Why can't they be module constants?" (PR #27809); "Is a single line needed to be abstracted into a function? What's the value add?" (PR #6725).
• Prefer plain module functions to single-method classes; avoid Mixins/util classes without a clear win; don't build for hypothetical futures.
• Delete dead/commented/debug code: "Clean this up please and write prod code." (PR #63805); "log don't print" (PR #54927); remove leftover print(row), commented blocks, debug logging.
• For AI-heavy or structural diffs, apply the full AI de-slop gate. Do not stop at identifying redundant individual lines when the package or service boundary is wrong.

6. Testing rigor (very common)

• Assert real values, not shapes. "We should be validating the actual values in the data frame too!! Just checking columns and data types trivializes the comparison" (PR #53258).
• Cover failure/edge paths, not just happy path: "Can we add test cases for when we raise the exception too?" (PR #55457); "test all potential paths … protect against incidental changes" (PR #27662).
• Stubs / in-memory fakes over mocks/patching. "pytest fixtures to mock remote DBs is for losers - cool kids use stub classes and clean InMem DAOs" (PR #52439); "I'm normally quite against using Mock explicitly. It looses explicit type enforcement and too easily enables poor code design" (PR #34166). (Aligns with the team rule: stubbed/parameterizable impls, no monkeypatching.)
• Keep code testable: inject time/deps, don't call now()/read env inside logic. "when you use ArboTimestamp.now(), you lose the ability to control the testing" (PR #30284).
• Use real proto objects over proto-text strings; randomize/parameterize inputs and column ordering.
• "where's the test?" — gate merges of critical logic on tests existing.
• For a launcher, service, poller, dashboard, artifact pipeline, or eval harness, require a real entrypoint smoke/e2e run in addition to unit tests. Verify traffic and output content, not merely process startup.

7. Layering, decoupling & globals (common, strongly held)

• Keep IO/DAOs out of pure logic; pass in only the minimal data needed. "OrderProposers should not maintain internal DAOs … should only take in the minimal information requirement to compute it's orders" (PR #22786).
• No globally shared object instances / module-import side effects / mutable arg defaults. "I'm always against globally shared object instances. This market calendar would be created on module import…" (PR #53011); "Don't create default objects in the args as this is created in the global space" (PR #29580); "we should almost never use global variables in python" (PR #29580).
• Production code must not import research/experimental code; modules shouldn't be "abused" into unrelated domains.
• Minimize coupling between systems ("I want to be minimally coupled with the Ledger here", PR #27738).

8. API / proto / interface design (common)

• Enums (not strings) for categorical/cross-team/FE-facing fields: "An Enum would allow for query aggregation against the reason" (PR #6567). Space enum magnitudes for forward-compat (PR #15323).
• Design messages for extensibility (oneof over Any, wrap core messages); flag schema smells ("synthetic data control permanently encoded into our proto schema", PR #66085).
• Wrap protos in domain objects (DomainProtocol with from_proto/to_proto); don't pickle for persistence — "pickling … not really meant for persistent storage. Ideally, we should have a proto and corresponding DomainProtocol" (PR #52891).

9. Naming & method semantics (common)

• Names must describe behavior: a function that builds a filter string shouldn't be named like it runs a read query (PR #3688); "exception_on_failure is not reflective of the function behaviour" (PR #34166). Suffix types with T, domain objects with D, IDs with Id.
• _-prefix internal-only members. classmethod/staticmethod for methods that don't touch instance state: "it's semantically incorrect to specify a method as instance-level if it doesn't access instance-level attributes" (PR #22914).

10. Imports, deps & hygiene (common)

• Imports at top of file, never inside functions. "why are we importing within a function?" (PR #26156). (He's noticed AI loves this: "function level import - why does AI love these?" PR #64561.)
• Keep Bazel deps matching actual imports; prune dead deps. Don't commit generated files, notebook/eval outputs, or secrets.
• After moves/deletes, scan for stale Python imports, shell paths, BUILD labels, runfiles, workflow strings, dynamic module registries, and facade exports. A green local import is not proof every entrypoint moved.

11. Comments & docs (common)

• Remove narrating comments ("# Setup logging"); write self-explanatory code instead. "Good code doesn't need comments as it's naturally readable" (PR #29580). Comments must explain why, not what; add docstrings/algorithm outlines for genuinely complex logic.
• Be skeptical of AI "slop": "are these LLM generated comments?" (PR #45792).
• Repository docs describe durable architecture or operation, not the agent's execution history. Reject handoff/progress/plan/change-log sprawl, but preserve owner-authored material when its ownership is ambiguous.

12. Modern era specifics (2025–2026)

• Don't change prompts/configs without an eval: "These changes feel arbitrary - did you run a batch evaluation to compare the performance?" (PR #49939). Keep general prompts generic; push specifics into function descriptions. Eval cases must be specific and gradeable.
• Triage automated security findings explicitly (Semgrep /fp with justification) rather than silently suppressing.

Code conventions

Write code so it would pass the checklist above. These are the patterns visible across his 1,140 authored PRs (Python-first; protobuf, Bazel, some TS).

Typing

• @dataclasses.dataclass(frozen=True) is the default domain primitive (module-qualified dataclasses.dataclass, not from dataclasses import dataclass). Add slots=True for hot-path configs; hand-define __hash__ when used as a dict/set key.
• Generics/TypeVar/Protocol for reusable infra (class DoFnBase(beam.DoFn, Generic[InT, OutT, FailedT])).
• ClassVar[...] for class constants; Literal/TypeAlias for closed string domains; Enum for closed sets; domain ID newtypes over str.
• Annotate every return type (including -> None). Prefer built-in generics + PEP 604 (list[str], dict[int, X], X | None) and from __future__ import annotations. # type: ignore only on untyped third-party imports; use precise cast(...) rather than loosening a signature.

Structure

• Explicit proto ↔ domain boundary: every proto gets a *_d.py domain dataclass with to_proto()/from_proto() (and empty_proto()) classmethods. Wire format stays out of logic.
• DAO layering: abstract base (ABC) → concrete (BQ…DB) → in-memory Fake…DB. IO lives behind DAOs; domain logic stays pure.
• Constructor dependency injection; store collaborators as _private attrs; factories for wiring.
• Hoist constants to module/class scope (UPPER_SNAKE), often derived from base units (30 * ONE_MIN). Maintain Bazel BUILD targets in lockstep with minimal, alphabetized deps.
• Shape directories around deployed services, stable libraries, targets, and recurring operator workflows. Do not use “generic” as a dumping ground for target/partner policy. Split files before they reach ~1k LOC.
• Public/private wrapper pairs: public process delegates to overridable _process. Decompose large functions; stage big features explicitly (e.g. "Direct Indexing (1/4): Protos").
• Suffix taxonomy: …D domain, …DB/…DAO persistence, _pb2 proto, …DoFn/…Transform, …Factory, …Reader/…Writer.

Error handling

• Fail-fast with raise + interpolated f"{var=}" messages; dedicated _validate_* methods.
• ValueError/custom exceptions for bad input; AssertionError only for "should-never-happen" invariants (often log-then-raise so regressions surface in CI). Exhaustive oneof dispatch via WhichOneof with an else: raise. Early-return guard clauses.

Testing

• Colocated *_test.py + matching py_test target, added in the same PR.
• Hand-written fakes/stubs injected in setUp; no mock.patch. Roundtrip tests (assertEqual(d.to_proto(), proto)), dataclasses.replace for variants, deterministic time helpers. Assert exact values/messages/structure and cover exception paths. Test names describe scenario + expected outcome; test private methods directly (test__foo).
• Exercise real launchers in their supported modes for service or orchestration changes. Record the concrete command, input/output counts, health/traffic evidence, persisted-contract checks, and cleanup.

Idioms

• f-strings everywhere (with = debug form and !r); comprehensions; dict-comprehension dedupe (list({a.name: a for a in attrs}.values())); dataclasses.field(default_factory=...) for mutable defaults; walrus for assign+check; @property for derived values; @contextmanager + try/finally for resources; typed flag wrappers (StringFlag/BoolFlag/ListFlag) plumbed end-to-end.
• Comments sparse but high-value (why/gotchas only). Quarantine experiments under experimental/<user>/.

PR descriptions

• Modern form: Conventional-Commit title (feat(scope):/fix(scope):), then ## Summary → ## Changes → ## Test plan with the exact bazel test … / pyright command pasted in. Link Jira tickets and Slack/PR context. For bug fixes, write Symptom → Root cause → Fix.

Before submitting

Run the repo's pre-PR sequence (see the pre-pr-checks skill): depclean → pyright → yapf → bazel build/bazel test, using gt commands when in a Graphite stack.

For structural or AI-heavy changes, also run a stale-path/reachability pass from real entrypoints, inspect the full changed-file inventory for one-offs/generated debris, verify every new flag has a caller, execute the affected service path locally, and confirm no scratch artifacts or processes remain. Follow the active worktree's execution guidance; do not run an expensive build merely for ritual when an approved Python-mode path provides the relevant validation.

Signature quotes (his voice, verbatim)

• "good code is easily testable code" (PR #35180)
• "Why is shadow a string? That feels very brittle" (PR #24963)
• "assert is for testing, raise exceptions" (PR #26666)
• "How do you know it's sorted? We should never assume external vendors will do anything correctly" (PR #29580)
• "pytest fixtures to mock remote DBs is for losers - cool kids use stub classes and clean InMem DAOs" (PR #52439)
• "I'm always against globally shared object instances." (PR #53011)
• "optimizing for ultimate extensibility is what made the old system so obtuse" (PR #44980)
• "We should be validating the actual values in the data frame too!! Just checking columns and data types trivializes the comparison" (PR #53258)
• "Bad pattern, everything should be explicitly matched no defaults" (PR #55510)
• "looping and comparing strings of keys to find an element is code smell. Have a little self respect" (PR #45792)
• "This seems crazy complex for what it's doing." (PR #26420)
• "this is a hack - we should fail on this" (PR #54643)
• "These changes feel arbitrary - did you run a batch evaluation to compare the performance?" (PR #49939)
• "Write self-explanatory code and remove comments" (PR #29580)
• "Clean this up please and write prod code." (PR #63805)
• "Always love to see 28k lines of code deleted" (PR #36405)
• "Kudos for your general test writing and documentation! You're very easy to build things with" (PR #26837)