szehon-ho commented on code in PR #57803: URL: https://github.com/apache/spark/pull/57803#discussion_r3752417720
########## AGENTS.md: ########## @@ -18,6 +18,8 @@ SQL golden file tests are managed by `SQLQueryTestSuite` and its variants. Read 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 members to an existing class or object, keep related kinds of code in sections rather than inserting into the middle of another section. A common layout is fields and constructors first, then the public/override API, then private helpers; in a test suite, tests together and helpers after them. Prefer appending to the matching section, or following the file's existing sectioning -- the common failure mode is inserting a new member next to its first use and splitting the surrounding section. Do not reorganize existing members unless the change requires it. Review Comment: Good call -- I dropped that clause. It turned out to describe a minority of suites, and the conflict argument holds up. I scanned every `*Suite.scala` under the test trees, split each file into top-level type bodies, and bucketed each non-lifecycle helper `def` by its position relative to the test blocks. 1,174 type bodies define at least one helper: | Helper placement | Share | |---|---:| | Above the first test | 49.2% | | Interleaved among the tests | 27.7% | | After the last test | 17.2% | | Split across both ends | 5.9% | So "helpers after the tests" was describing about one suite in six. And as you point out, since new tests get appended at the end, helpers parked there sit on exactly the lines new tests want. Broken down by suite size (n = 900 small, 274 large): | Helper placement | Small (<20 tests) | Large (20+ tests) | |---|---:|---:| | Above the first test | 55.6% | 28.5% | | Interleaved among the tests | 17.7% | 60.6% | | After the last test | 20.1% | 7.7% | | Split across both ends | 6.7% | 3.3% | Small suites overwhelmingly put helpers at the top; large ones mostly interleave, keeping a helper beside the group of tests that uses it. Probably two things going on: in a multi-thousand-line suite a single top block is impractical to read against, and large suites also accumulate edits from many contributors over the years, so they drift rather than following any one convention. The style guides turned out to say less than I had assumed. Spark's contributing page points at the official Scala style guide and the Databricks guide, preferring the latter. Databricks has a single sentence on this, under "Ordering within a Class": *"If a class is long and has many methods, group them logically into different sections, and use comment headers to organize them."* No ordering beyond that. The official Scala guide's "Ordering Of Class Elements" adds only *"Fields should precede methods in a scope."* Neither mentions public-before-private, and neither says anything about test helpers. Nothing in scalastyle or scalafmt enforces member order either. So I cut the ordering I had made up and kept only the section-grouping principle you suggested, which is the part the Databricks guide actually states: > When adding a member to an existing class or object, follow the sectioning the file already uses and put the new member with the code it belongs with. The common failure mode is dropping it wherever it is first used without checking how the file is organized, splitting a section of unrelated code in the process. Beyond grouping related code there is no prescribed order -- the Databricks Scala guide, which Spark follows, asks only that a long class group its members into logical sections with comment headers. Do not reorganize existing members unless the change requires it. I also reworded the failure mode. Calling it "inserting the member next to its first use" contradicted both the first sentence, since a caller is related code, and the data above, since placing a helper beside the tests it serves is the majority pattern in large suites. The real problem is placing a member without looking at how the file is organized, so that it splits an unrelated section. -- 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]
