Miretpl commented on code in PR #73359:
URL: https://github.com/apache/airflow/pull/73359#discussion_r4054062220
##########
.github/instructions/code-review.instructions.md:
##########
@@ -65,7 +65,13 @@ Flag these patterns that indicate low-quality AI-generated
contributions:
- **Description doesn't match code**: PR description describes something
different from what the code actually does.
- **No evidence of testing**: Claims of fixes without test evidence, or author
admitting they cannot run the test suite.
- **Over-engineered solutions**: Adding caching layers, complex locking, or
benchmark scripts for problems that don't exist or are misunderstood.
-- **Narrating or redundant comments**: Comments that restate what the next
line does (e.g., `# Add the item to the list` before `list.append(item)`);
multi-line prose explaining a one-line change; the same rationale repeated at
several sites; or explanatory comments on tests whose names already convey
intent. Comments should explain *why* when it is non-obvious, not narrate
*what*. Flag over-commenting as noise to be trimmed.
+- **Narrating or redundant comments**: Flag comments that restate code or test
names, duplicate
+ rationale, or explain only a generic purpose (e.g., `# Log for debugging`
above `logging.info(...)`).
+ Explaining "why" is insufficient unless the comment preserves specific
context that is not
+ readily apparent from the code. Identify the misunderstanding or incorrect
change it prevents;
+ if removing it loses no such context, flag it for removal. A subtle one-line
change can still
+ warrant an explanation of a constraint or failure mode. Apply the
+ [comment guidance in AGENTS.md](../../AGENTS.md#coding-standards).
Review Comment:
```suggestion
- **Non-contextual or narrating comments**: Flag comments that restate code
or test names, duplicate
rationale, or explain generic purpose (e.g., `# Log for debugging` above
`logging.debug(...)`).
Explaining "why" is insufficient unless the comment preserves specific
context that is not
readily apparent from the code itself. Identify the misunderstanding or
incorrect change the comment prevents.
If removing a comment does not lose context, flag it for removal. A
one-line change can still
warrant an explanation of a constraint or failure mode. Apply the
[comment guidance in AGENTS.md](../../AGENTS.md#coding-standards).
```
Similar to the comment above.
##########
AGENTS.md:
##########
@@ -134,7 +134,18 @@ reported as such are described in "What is NOT considered
a security vulnerabili
- **Always format and check Python files with ruff immediately after writing
or editing them:** `uv run ruff format <file_path>` and `uv run ruff check
--fix <file_path>`. Do this for every Python file you create or modify, before
moving on to the next step.
- No `assert` in production code.
-- **Comment sparingly — code says *what*, comments say *why*.** Add a comment
only when the reasoning is non-obvious and cannot be carried by a clear name or
the code itself. Do not write narrating comments that restate the next line, do
not pad logic with multi-line prose, and do not repeat the same rationale at
several sites — put one concise note at the source of truth and let the others
stand on their own. Tests whose names already describe intent need no
explanatory comment. Reserve longer explanation for genuinely complex or
non-obvious logic (e.g. a security check whose threat model isn't apparent),
and keep even that as tight as it can be. Over-commenting is noise that ages
badly and obscures the code it wraps.
+- **Add a comment only when it preserves context that is not readily apparent
from the code.**
+ Explaining a generic purpose does not qualify: `# Log for debugging` above
`logging.info(...)`,
+ `# Validate for safety`, and `# Retry for reliability` add no useful
information.
+ Useful comments record a specific constraint, invariant, compatibility
quirk, or tradeoff;
+ for example, `# Closing the connection clears the request ID, so log first.`
is useful
+ if that constraint actually applies and is not apparent at the call site.
+ Before adding a comment, identify the misunderstanding or incorrect change
it would prevent.
+ If removing it loses no such context, omit it. Prefer clearer names or
structure when they
+ convey the same information. Do not narrate code, repeat test names, or
invent a rationale.
+ Keep necessary explanations concise and at the source of truth; do not
repeat them at each
+ call site. Judge their value by the context they preserve, not by the number
of lines of code
+ they describe.
Review Comment:
```suggestion
- **Comment only when context is not readily apparent from the code.**
Generic purpose explanation do not qualify: `# Log for debugging` above
`logging.debug(...)`,
`# Validate for safety`, and `# Retry for reliability` and similar,
provide no useful information.
Useful comments record a specific constraint, invariant, compatibility
quirk, or tradeoff.
Before adding a comment, identify the misunderstanding or incorrect change
it would prevent.
If removing a comment will lose non-code-related context, omit it. Do not
narrate code, repeat code,
or invent a rationale in comments. Keep necessary explanations short,
precise and at the source of truth.
Do not repeat comments at each call site. Judge each comment value by the
preserved context only.
```
Removal of `# Closing the connection clears the request ID, so log first.`
example, as without more context, it is not really clear what is wrong with
that kind of comment and placing the whole code example probably is not needed.
Deleting of `Prefer clearer names or structure when they provide the same
information.` as I personally don't know how to precisely understand it by
looking at sentences before and after, but I'm also not sure about that fully.
The rest of the changes are mostly language edits to be more precise and
clearer (at least for my understanding of English and how
context/multidimentional spaces work).
Do we have any CI checked for checking how changes like that will impact the
models?
--
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]
