Skip to content

[MINOR][DOC] Suggest grouping code into sections when adding class members in AGENTS.md - #57803

Open
szehon-ho wants to merge 2 commits into
apache:masterfrom
szehon-ho:agents-member-placement
Open

[MINOR][DOC] Suggest grouping code into sections when adding class members in AGENTS.md#57803
szehon-ho wants to merge 2 commits into
apache:masterfrom
szehon-ho:agents-member-placement

Conversation

@szehon-ho

@szehon-ho szehon-ho commented Aug 6, 2026

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Adds one paragraph to the ## Development Notes section of AGENTS.md on where to place new members when editing an existing class or object: keep related kinds of code in sections and append a new member to the matching section, rather than inserting it into the middle of another section. It sketches a common layout (fields and constructors, then the public/override API, then private helpers; in a test suite, tests together with helpers after them) and names the failure mode it is meant to prevent -- inserting a new member next to its first use and splitting the surrounding section.

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 defers to the file's existing sectioning and explicitly does not ask anyone to reorganize existing code.

Why are the changes needed?

Agents writing code top-down tend to define a member immediately above its first use, 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 code 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:

git diff -U0 -- AGENTS.md | grep '^+' | grep -v '^+++' | grep -P "[^\x00-\x7F]"

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Cursor (Opus 5)

@szehon-ho

Copy link
Copy Markdown
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

@uros-b

uros-b commented Aug 6, 2026

Copy link
Copy Markdown
Member

I don't have a strong opinion, but it makes sense to me and can be nicer to have this, so +1

Comment thread AGENTS.md Outdated

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we turn it into a more general code placement principle? e.g. we should group the code into sections

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea -- updated to a more general section-grouping principle: keep related kinds of code in sections and append a new member to the matching section, rather than a rule specific to private helpers. Kept a short note on the common failure mode of inserting a member next to its first use and splitting the surrounding section.

Address review feedback: state the general principle of grouping members
into sections and appending to the matching one, instead of a rule
specific to private helpers.
@szehon-ho szehon-ho changed the title [MINOR][DOC] Suggest where to place new class members in AGENTS.md [MINOR][DOC] Suggest grouping code into sections when adding class members in AGENTS.md Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants