Unify @return KDoc style across the public API#256
Conversation
|
|
||
| /** | ||
| * Returns an immutable list containing all elements of this collection. | ||
| * Converts this collection to an immutable list. |
There was a problem hiding this comment.
It's rather iterable, not a collection
There was a problem hiding this comment.
Good catch — fixed in d696b4d. The same slip applied to Iterable<T>.toPersistentList(), toImmutableSet(), toPersistentSet(), and toPersistentHashSet() (all use Iterable<T> as receiver but said "this collection" in the KDoc), so I fixed all five together.
| public interface PersistentList<out E> : ImmutableList<E>, PersistentCollection<E> { | ||
| /** | ||
| * Returns a new persistent list with the specified [element] appended. | ||
| * Returns the result of appending the specified [element] to this list. |
There was a problem hiding this comment.
Could you elaborate why do you think "Returns the result of ..." version is better than what was written before?
Previous version was explicit about the new list being returned, and the new version gives an impression that the list will be modified.
Also, "Returns the result of ..." looks excessive, it makes sense to describe an operation in terms of actions and it should be obvious that it returns the result of that action.
There was a problem hiding this comment.
Thanks — both points are well taken. Let me explain the motivation first, then propose two refinements.
My goal was to apply one rule across the public API: every non-Unit function should carry an @return tag. To avoid the tag being a verbatim echo of the summary, I rephrased summaries so the two would read complementarily. The Returns the result of <V-ing> … shape wasn't invented in this PR — it's the dominant style for operator overloads in extensions.kt on master (three PersistentMap.minus(keys: …) overloads, lines 495–520; ~42 KDoc blocks across PersistentCollection, PersistentList, PersistentSet, and PersistentMap operators use the same shape). I extrapolated from that precedent.
That said, you're right on both counts. For persistent collections, copy semantics is the invariant we want to be unambiguous about, and Returns the result of appending [element] to this list reads uncomfortably close to mutation of the receiver. And Returns the result of <V-ing> … is redundant when the operation description already implies what's returned.
Two refinements come to mind, depending on which unification target you'd prefer:
A. Drop @return where it would only echo the summary. Restore the original Returns a new persistent list with the specified [element] appended. as the sole description, and add @return only when it carries information beyond the summary (e.g., or this instance if no modifications were made). Unification then reads as "every non-Unit function's return value is unambiguously documented", not "every function has an @return tag".
B. Keep @return on every non-Unit function as in this PR, but switch the summary to an action-verb form: Appends the specified [element] to this list. rather than Returns the result of appending …. The participial method names from #233 (adding, removing, cleared) plus the explicit new persistent list in @return should keep copy semantics unambiguous.
A is simpler and closer to the Kotlin stdlib idiom (Returns a list containing all elements of the original collection); B preserves the universal-@return invariant. Which direction would you prefer?
There was a problem hiding this comment.
One more option, on reflection:
C. Drop @return entirely from the library's public docs and fold return info (including conditional branches) into the summary as a single sentence:
/**
* Returns a new persistent map with the specified [keys] and their corresponding values removed,
* or this instance if no modifications were made in the result of this operation.
*/Closest to the Kotlin stdlib idiom, and addresses both your concerns directly — "Returns a new persistent X with Y" stays in the summary (no mutation hint), and there's no @return to be verbose about. Trade-off: a more substantial rewrite of master, and it reinterprets the #254 target as "consistent absence of @return" rather than "consistent presence".
The receivers of `toImmutableList`, `toPersistentList`, `toImmutableSet`, `toPersistentSet`, and `toPersistentHashSet` on `Iterable<T>` are iterables, not collections. KDoc summaries and `@return` tags previously called them "this collection" — corrected to "this iterable" in both places (12 lines across 5 functions). Addresses #256 (comment) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| * Creates an empty persistent list. | ||
| * | ||
| * @return an empty persistent list. |
There was a problem hiding this comment.
Lots of these are just noise. I don't think many of these changes offer an actual improvement.
https://publicobject.com/2007/08/05/stop_writing_redundant_javadoc/
Apply a single `@return` convention library-wide: every public function that returns a non-Unit value carries an `@return` tag, continuation lines align 8 spaces past the leading ` * ` (matching the existing style in the `Immutable*.kt` interfaces), and main descriptions are rephrased (`Creates ...`, `Converts ...`, `Returns the result of <V-ing> ...`) where adding `@return` would otherwise produce a verbatim duplicate. Resolves the inconsistency surfaced in PR #233 review where `PersistentList`-receiving operator overloads in `extensions.kt` lacked `@return` while sibling operators on `PersistentCollection`, `PersistentSet`, and `PersistentMap` carried it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The receivers of `toImmutableList`, `toPersistentList`, `toImmutableSet`, `toPersistentSet`, and `toPersistentHashSet` on `Iterable<T>` are iterables, not collections. KDoc summaries and `@return` tags previously called them "this collection" — corrected to "this iterable" in both places (12 lines across 5 functions). Addresses #256 (comment) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
d696b4d to
2a03547
Compare
Summary
Resolves #254. Apply a single
@returnKDoc convention library-wide across the five public-API files incore/commonMain/src/:Unitvalue carries an@returntag.*— matches the existing style inImmutable*.ktinterfaces.@returnwould have produced a verbatim duplicate of the main description, the main description is rephrased (Creates …,Converts …,Returns the result of <V-ing> …) so the two read as complementary.This addresses the inconsistency surfaced in the #233 review, where
PersistentList-receiving operator overloads inextensions.kt(e.g.PersistentList<E>.plus(element)) lacked@returnwhile sibling operators onPersistentCollection,PersistentSet, andPersistentMapcarried it.Files touched:
core/commonMain/src/extensions.ktcore/commonMain/src/ImmutableCollection.ktcore/commonMain/src/ImmutableList.ktcore/commonMain/src/ImmutableMap.ktcore/commonMain/src/ImmutableSet.ktThis is a docs-only change: no public API or behavior is affected.
Test plan
./gradlew :kotlinx-collections-immutable:compileKotlinJvm :kotlinx-collections-immutable:compileKotlinMetadata— succeeds./gradlew :kotlinx-collections-immutable:apiCheck— succeeds (no API change)./gradlew :kotlinx-collections-immutable:dokkaGenerate— succeeds with no KDoc warningsPersistentList<E>.plus(element), one factory (e.g.persistentListOf), and one converter (e.g.Iterable<T>.toImmutableList) to confirm@returnrenders correctly🤖 Generated with Claude Code