mbutrovich commented on code in PR #3337:
URL: https://github.com/apache/iceberg-rust/pull/3337#discussion_r4188649116
##########
AGENTS.md:
##########
@@ -22,6 +22,33 @@
This file provides repository-specific guidance for automated agents working
in this repository.
+## Comments, docs, and diff scope
+
+Default to no comment. Leave existing comments as they are unless you change
+the code they describe or they are wrong.
+
+- Comment only what the code cannot say: invariants, non-obvious
+ constraints, links to external behavior. Do not restate the code.
+- Do not describe how the code changed or explain review history
+ ("previously", "instead of", "unlike X"). Put that in the commit message
+ or PR description.
+- Public items get a one-line doc summary of what they do for the caller,
+ not how. Expand only for contract the caller must know.
Review Comment:
Should this name the contract a caller must know? The Java guidance this
follows lists it explicitly ("Document thrown exceptions, null behavior, and
resource ownership",
[AGENTS.md](https://github.com/apache/iceberg/blob/17d375a0ed3a9213935f59e04404b83c64e982a6/AGENTS.md?plain=1#L117)).
The Rust equivalent is the `# Errors` and `# Panics` sections from the [Rust
API
guidelines](https://rust-lang.github.io/api-guidelines/documentation.html#function-docs-include-error-panic-and-safety-considerations-c-failure),
and the crates already have 31 `# Errors` sections. As I read the current
wording, an agent following "one-line doc summary" could drop them from new
public functions.
```suggestion
- Public items get a one-line doc summary of what they do for the caller,
not how. Expand only for contract the caller must know, such as the
errors it returns (`# Errors`) or when it panics (`# Panics`).
```
##########
AGENTS.md:
##########
@@ -22,6 +22,33 @@
This file provides repository-specific guidance for automated agents working
in this repository.
+## Comments, docs, and diff scope
+
+Default to no comment. Leave existing comments as they are unless you change
+the code they describe or they are wrong.
+
+- Comment only what the code cannot say: invariants, non-obvious
+ constraints, links to external behavior. Do not restate the code.
+- Do not describe how the code changed or explain review history
+ ("previously", "instead of", "unlike X"). Put that in the commit message
+ or PR description.
+- Public items get a one-line doc summary of what they do for the caller,
+ not how. Expand only for contract the caller must know.
+- Test names say what they check. Do not narrate asserts.
+- Do not add tests that duplicate existing coverage.
+- Do not rename, reformat, or reorder code unrelated to the change.
Review Comment:
Should this also cover refactors and API changes? A bug fix that carries an
API redesign is the scope creep I see most often from agents, and rename,
reformat, and reorder don't name it.
```suggestion
- Do not rename, reformat, refactor, or reorder code unrelated to the
change. A bug fix does not carry an API redesign.
```
##########
AGENTS.md:
##########
@@ -22,6 +22,33 @@
This file provides repository-specific guidance for automated agents working
in this repository.
+## Comments, docs, and diff scope
+
+Default to no comment. Leave existing comments as they are unless you change
+the code they describe or they are wrong.
+
+- Comment only what the code cannot say: invariants, non-obvious
+ constraints, links to external behavior. Do not restate the code.
+- Do not describe how the code changed or explain review history
+ ("previously", "instead of", "unlike X"). Put that in the commit message
+ or PR description.
+- Public items get a one-line doc summary of what they do for the caller,
+ not how. Expand only for contract the caller must know.
+- Test names say what they check. Do not narrate asserts.
+- Do not add tests that duplicate existing coverage.
+- Do not rename, reformat, or reorder code unrelated to the change.
+- Reuse existing helpers and types before adding new ones.
Review Comment:
Could this name the two places agents most often miss? One is test
scaffolding, where
[`TableTestFixture`](https://github.com/apache/iceberg-rust/blob/df4975b930533eb0c4a2535e16e9e206e64835f1/crates/iceberg/src/test_utils/scan.rs#L77)
already covers most scan setup. The other is arrow-rs, where agents tend to
hand-write per-row loops that an existing kernel already does.
```suggestion
- Reuse existing helpers, types, and test fixtures (such as
`TableTestFixture`) before adding new ones. Use arrow-rs kernels
instead of reimplementing them.
```
##########
AGENTS.md:
##########
@@ -22,6 +22,33 @@
This file provides repository-specific guidance for automated agents working
in this repository.
+## Comments, docs, and diff scope
+
+Default to no comment. Leave existing comments as they are unless you change
+the code they describe or they are wrong.
+
+- Comment only what the code cannot say: invariants, non-obvious
+ constraints, links to external behavior. Do not restate the code.
+- Do not describe how the code changed or explain review history
+ ("previously", "instead of", "unlike X"). Put that in the commit message
+ or PR description.
+- Public items get a one-line doc summary of what they do for the caller,
+ not how. Expand only for contract the caller must know.
+- Test names say what they check. Do not narrate asserts.
+- Do not add tests that duplicate existing coverage.
Review Comment:
Both test rules here push toward fewer tests. What do you think about adding
a line that keeps edge cases and error paths in scope, so agents don't read
this as "test only the happy path"? Asserting against a whole expected value
(for example a full `Schema`) instead of checking fields one at a time also
fits the goal of this PR, since it makes tests shorter and their failures
easier to read.
```suggestion
- Test names say what they check. Do not narrate asserts.
- Cover the edge cases and error paths the change adds, not only the
happy path.
- Build the expected value and assert equality with it, rather than
checking fields one at a time.
- Do not add tests that duplicate existing coverage.
```
##########
AGENTS.md:
##########
@@ -22,6 +22,33 @@
This file provides repository-specific guidance for automated agents working
in this repository.
+## Comments, docs, and diff scope
+
+Default to no comment. Leave existing comments as they are unless you change
+the code they describe or they are wrong.
+
+- Comment only what the code cannot say: invariants, non-obvious
+ constraints, links to external behavior. Do not restate the code.
+- Do not describe how the code changed or explain review history
+ ("previously", "instead of", "unlike X"). Put that in the commit message
+ or PR description.
+- Public items get a one-line doc summary of what they do for the caller,
+ not how. Expand only for contract the caller must know.
+- Test names say what they check. Do not narrate asserts.
+- Do not add tests that duplicate existing coverage.
+- Do not rename, reformat, or reorder code unrelated to the change.
+- Reuse existing helpers and types before adding new ones.
+- Use the narrowest visibility that works. Make items `pub` only when they
+ are part of the API the change intends to expose; `public-api.txt`
+ changes show API growth.
+
+## Pull requests
+
+- Follow the PR template. Keep each section short: the problem, the
+ approach, and anything a reviewer would not guess from the diff.
Review Comment:
What do you think about saying what goes in the AI Disclosure section? The
[Iceberg AI contribution
guidelines](https://github.com/apache/iceberg/blob/17d375a0ed3a9213935f59e04404b83c64e982a6/site/docs/contribute.md?plain=1#L67)
that the template links ask authors to "call out any areas of uncertainty or
assumptions ... so reviewers can focus on them." With "keep each section short"
and nothing else, I'd expect agents to fill that section with a one-line tool
name, which drops the part reviewers use.
```suggestion
- Follow the PR template. Keep each section short: the problem, the
approach, and anything a reviewer would not guess from the diff. In
AI Disclosure, name the areas of uncertainty or assumptions a reviewer
should focus on.
```
##########
AGENTS.md:
##########
@@ -22,6 +22,33 @@
This file provides repository-specific guidance for automated agents working
in this repository.
+## Comments, docs, and diff scope
+
+Default to no comment. Leave existing comments as they are unless you change
+the code they describe or they are wrong.
+
+- Comment only what the code cannot say: invariants, non-obvious
+ constraints, links to external behavior. Do not restate the code.
+- Do not describe how the code changed or explain review history
+ ("previously", "instead of", "unlike X"). Put that in the commit message
+ or PR description.
+- Public items get a one-line doc summary of what they do for the caller,
+ not how. Expand only for contract the caller must know.
+- Test names say what they check. Do not narrate asserts.
+- Do not add tests that duplicate existing coverage.
+- Do not rename, reformat, or reorder code unrelated to the change.
+- Reuse existing helpers and types before adding new ones.
+- Use the narrowest visibility that works. Make items `pub` only when they
+ are part of the API the change intends to expose; `public-api.txt`
+ changes show API growth.
Review Comment:
Should this tell the agent how `public-api.txt` gets updated? Each published
crate has its own file, generated by [`make
generate-public-api`](https://github.com/apache/iceberg-rust/blob/df4975b930533eb0c4a2535e16e9e206e64835f1/Makefile#L66-L73)
and checked in CI by `make check-public-api`. Without the command, an agent
may hand-edit the file or leave it stale and fail CI.
```suggestion
- Use the narrowest visibility that works. Make items `pub` only when they
are part of the API the change intends to expose. Run
`make generate-public-api` to update each crate's `public-api.txt`, and
check its diff for API growth.
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
