mchades commented on code in PR #11339:
URL: https://github.com/apache/gravitino/pull/11339#discussion_r3341751319
##########
AGENTS.md:
##########
@@ -42,10 +43,64 @@
4. Constructors.
5. Methods (Group by visibility, putting `private` methods at the end).
+## Code Design & Structure
+- **Method granularity**: Each method must earn its existence. Avoid small,
fragmented single-use helpers — keep cohesive logic together. At the same time,
fold genuinely duplicated logic into one shared helper instead of repeating it.
Extract when it is reused or names a genuinely non-obvious step; inline trivial
one-liners.
+- **No dead parameters or methods**: Remove a parameter that is always the
same constant (for example, a flag that is always `true`) and a method that
does nothing meaningful. Keep related declarations and their setup together
(for example, a thread-pool field and the loop that submits tasks to it).
+- **Field assignment**: Qualify field assignments with `this.` (for example,
`this.workers = ...`) in constructors and setters.
+- **Test-only code must not live in production classes**: Never add a
production method or field that exists only to serve a test.
+ - Prefer keeping test-only logic in the test module (test helpers, fixtures,
or the test class itself).
+ - If a production member genuinely must be reachable from a test, annotate
it with `@VisibleForTesting` and keep the narrowest visibility possible
(package-private, not `public`/`protected`). Never widen visibility solely for
a test.
+ - Do not reach into non-public members from tests via reflection
(`setAccessible(true)`). Add a `@VisibleForTesting` constructor/factory instead.
+- **Narrowest visibility**: Give every class member the smallest visibility
that works (`private` first, then package-private). Do not make something
`public`/`protected` unless an external caller needs it.
+- **Dependency injection, not singletons**: Avoid the singleton pattern
(including lazy-holder/double-checked instances) — it is hard to test.
Construct collaborators once in the owning class (for example, the
plugin/bootstrap class) and inject them; do not `new` a manager/service inline
in the middle of logic, and never create two instances of a component that is
meant to be shared.
+- **Naming follows existing conventions**: Match the naming patterns already
used by sibling classes and the surrounding package, and make the name reflect
the class's actual role. Established suffixes include `XxxManager`,
`XxxDispatcher`, `XxxService`, `XxxOperations`, `XxxListener`, `XxxPoller`,
capability interfaces `SupportsXxx`, and tests `TestXxx`. Capitalize acronyms
consistently with existing code (`RESTUtils`, not `RestUtils`). For boolean
fields and accessors, follow the surrounding module's established convention
and keep the field name consistent with its getter semantics. Do not invent a
new convention when a matching one already exists.
+- **Names must reveal behavior**: Use domain terms that make the behavior
clear. Do not name a mutating or state-transitioning method like a read-only
getter (`getXxx`), and avoid vague jargon when a concrete term such as
`finished`, `cleanup`, or `runnable` is clearer.
+- **Reuse existing types and patterns**: Before adding a new request DTO, PO,
or helper, check whether an existing one (for example, an `XxxAssociateRequest`
or `XxxPO`) already fits, and reuse it. Validate request DTOs at the request
boundary (reject nulls, overlapping add/remove sets, etc.).
+- **Prefer immutable data objects**: Keep DTOs and POs immutable where
practical. Prefer constructors or builders over setters unless a framework
requires mutable JavaBeans-style objects.
+- **Exceptions**: Throw the standard, semantically correct exception type for
the situation (for example, the project's unauthorized/forbidden exception for
auth failures). Do not add a `catch (RuntimeException e) { throw e; }` that
adds no behavior, and collapse identical sibling `catch` blocks. When
refactoring, preserve identifying context (such as the table identifier) in
error messages and logs.
+- **Prefer standard-library and Commons idioms**: Use
`StringUtils.isBlank`/`isNotBlank` instead of manual null/empty string checks,
a `Set` (not a `List`) for repeated membership lookups, and
`Base64.getEncoder().encodeToString(...)` rather than manual byte/string
round-trips. Do not introduce calls to APIs that are deprecated at the version
the build pins; keep a `@SuppressWarnings("deprecation")` only when it is
required for compatibility, with a comment explaining why.
+- **Module and package responsibilities**: Keep module and package boundaries
explicit. Before adding a class, inspect sibling classes and place it in the
module and package that own its concern. Do not put a class in a convenient but
unrelated package, and do not bypass the owning abstraction to reach an
incidental service or implementation layer.
+- **Component placement & lifecycle**: A sub-component of another component
(for example, a side module of the entity store) should be created, started,
and closed by its owner — mirroring sibling components such as
`RelationalGarbageCollector` — rather than being exposed and wired as a global
`GravitinoEnv` component.
+- **Avoid speculative abstractions**: Do not add an SPI, factory, helper, or
extension point without a concrete current need. Prefer the smallest
implementation that satisfies today's callers, then generalize when a second
real use case requires it.
+- **Prefer capability interfaces over concrete casts**: Detect optional
behavior through a `SupportsXxx` capability interface plus `instanceof`, not by
casting to a concrete implementation class.
+- **Argument validation**: Use `Preconditions.checkArgument(condition,
message)` for argument/state validation; do not use `checkNotNull` (it throws
`NullPointerException` rather than the more meaningful
`IllegalArgumentException`). Only null-check values that can actually be null —
do not add defensive null checks for values guaranteed non-null by their source
(for example, rows returned by a MyBatis mapper).
Review Comment:
This guidance appears to conflict with our existing instruction in
.github/copilot-instructions.md that says to always check for nulls.
Could we align the wording here to avoid inconsistent review guidance?
--
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]
