This is an automated email from the ASF dual-hosted git repository.
Jackie-Jiang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new bd5cf5e6d47 Clarify review compatibility and style conventions (#20184)
bd5cf5e6d47 is described below
commit bd5cf5e6d4749470a31d80d889290371ca4944d1
Author: Xiaotian (Jackie) Jiang <[email protected]>
AuthorDate: Fri Nov 6 17:35:19 2026 -0800
Clarify review compatibility and style conventions (#20184)
---
kb/code-review-principles.md | 23 +++++++++++++++++++----
kb/skills/review-naming-api.md | 4 +++-
2 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/kb/code-review-principles.md b/kb/code-review-principles.md
index 79c182cc04d..e250340f89f 100644
--- a/kb/code-review-principles.md
+++ b/kb/code-review-principles.md
@@ -124,6 +124,10 @@ Original scope of a flag must be understood before
extending to new behavior.
**C1.10 — Backward-compatibility mechanisms must have a sunset plan**
Shims should be scrutinized for ongoing operational cost and have a clear
removal date.
+Distinguish Java constructor compatibility from mixed-version JSON
compatibility before adding overloads.
+Retain legacy constructors only when they protect a supported Java
source/binary API contract; public visibility alone
+does not make an internal DTO such a contract. Preserve mixed-version JSON
compatibility independently, including
+missing-field defaults and unknown-field handling.
- Trigger: Any PR adding backward-compat shims or fallback code paths
**C1.11 — Default value changes require production testing confidence**
@@ -1149,7 +1153,9 @@ Import `FieldSpec.DataType` and
`DataSchema.ColumnDataType` directly. Use `DataT
instead of the enclosing-class-qualified names.
- Trigger: Any PR using `FieldSpec.DataType` or `DataSchema.ColumnDataType`
outside import declarations
-**C7.23 — Put multiline ternary branches on separate lines**
+**C7.23 — Ternary condition and formatting conventions**
+For nullable-value ternaries, prefer the non-null branch first: `value != null
? value.method() : null`.
+See C5.21 before adding a null guard for a constructor-enforced invariant.
Keep a ternary expression on one line when it fits. Otherwise, put the true
and false branches on separate lines:
```java
@@ -1158,7 +1164,16 @@ condition
: falseExpression;
```
-- Trigger: Any PR adding or modifying a multiline ternary expression
+- Trigger: Any PR adding or modifying a ternary expression
+
+**C7.24 — Keep getter order consistent with field declarations**
+Keep getters in field-declaration order, matching constructor parameter and
assignment order where those already align.
+- Trigger: Any PR adding or moving getters in a class with an established
field/accessor order
+
+**C7.25 — Avoid consecutive blank lines inside class bodies**
+Use at most one blank line between members in touched code. Preserve the
repository's two-blank-line separator between
+imports and top-level class documentation.
+- Trigger: Any PR adding or touching class members with consecutive blank lines
---
@@ -1293,6 +1308,6 @@ Approve the immediate fix while deferring broader design
to separate threads.
| 4. Performance & Efficiency | 2 | 16 | 4 | 22 |
| 5. Correctness & Safety | 9 | 16 | 3 | 28 |
| 6. Testing Strategies | 2 | 7 | 9 | 18 |
-| 7. Naming & API Design | 1 | 9 | 13 | 23 |
+| 7. Naming & API Design | 1 | 9 | 15 | 25 |
| 8. Process & Scope | 2 | 10 | 8 | 20 |
-| **Total** | **32** | **83** | **52** | **167** |
+| **Total** | **32** | **83** | **54** | **169** |
diff --git a/kb/skills/review-naming-api.md b/kb/skills/review-naming-api.md
index e657acfbc92..009d7254112 100644
--- a/kb/skills/review-naming-api.md
+++ b/kb/skills/review-naming-api.md
@@ -14,7 +14,9 @@ impact; pattern matches are investigation triggers, not
findings or automatic se
- Inline fully-qualified class names — flag (CLAUDE.md convention).
- Qualified `Assert.` or `Mockito.` method calls in test files — apply C7.21.
- `FieldSpec.DataType` or `DataSchema.ColumnDataType` outside import
declarations — apply C7.22.
-- Multiline ternary expressions — verify C7.23 formatting.
+- Added or modified ternary expressions — apply C7.23 condition and formatting
conventions.
+- Added or moved getters — verify field-declaration order per C7.24.
+- Consecutive blank lines in touched class bodies — apply C7.25; preserve the
import/class-documentation separator.
- New public classes without class-level Javadoc — flag (CLAUDE.md convention).
- License headers on new files — flag missing.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
