[MINOR][DOC] Suggest where to place new class members in AGENTS.md - #57803
Open
szehon-ho wants to merge 1 commit into
Open
[MINOR][DOC] Suggest where to place new class members in AGENTS.md#57803szehon-ho wants to merge 1 commit into
szehon-ho wants to merge 1 commit into
Conversation
Generated-by: Cursor (Opus 5)
Member
Author
|
@cloud-fan @HyukjinKwon do you have any thought on it? i sometimes find annoying the ai generate a private method and put it on top |
HyukjinKwon
approved these changes
Aug 6, 2026
uros-b
approved these changes
Aug 6, 2026
Member
|
I don't have a strong opinion, but it makes sense to me and can be nicer to have this, so +1 |
cloud-fan
reviewed
Aug 6, 2026
|
|
||
| Spark Connect protocol is defined in proto files under `sql/connect/common/src/main/protobuf/`. Read the README there before modifying proto definitions. | ||
|
|
||
| When adding a member to an existing class, prefer a position that does not split up what is already there. A good default for a new private helper is after the public methods that use it, or alongside the file's existing helpers; in a test suite, after the `test(...)` blocks rather than between them. The common failure mode is placing a helper immediately above the first method that uses it, which interrupts a top-to-bottom read of the class's public surface or of the suite's list of cases. Conventions vary between files, so follow the local one rather than reorganizing. |
Contributor
There was a problem hiding this comment.
shall we turn it into a more general code placement principle? e.g. we should group the code into sections
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Adds one paragraph to the
## Development Notessection ofAGENTS.mdsuggesting where to place a new member when editing an existing class: a private helper is better placed after the public methods that use it, or with the file's existing helpers, rather than inserted between two public methods or between twotest(...)blocks.It is phrased as a suggestion with a default, not a rule. Files in the repo vary in how they order members, so the note explicitly defers to the local convention of whatever file is being edited and does not ask anyone to reorganize existing code.
Why are the changes needed?
Agents writing code top-down tend to define a helper immediately above the first method that uses it, which lands it in the middle of a class's public surface or a suite's list of test cases and interrupts a top-to-bottom read. I hit exactly this while writing a test suite and had to move the helper in review.
This fits the existing pattern of the surrounding notes, which call out specific, recurring failure modes (non-ASCII characters in comments, overrunning the 100-character line limit) rather than restating general style guidance. Unlike those two, this one has no linter behind it, so the note is the only signal available.
Does this PR introduce any user-facing change?
No. Documentation for contributors and agent tooling only.
How was this patch tested?
No tests; documentation only. Verified the added text is pure ASCII, per the rule in the paragraph immediately below it:
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Cursor (Opus 5)