KDocs for min/max - #2012
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as low quality.
This comment was marked as low quality.
AndreiKingsley
left a comment
AndreiKingsley
left a comment
There was a problem hiding this comment.
I am not sure about maxBy/maxFor descriptions — they look complicated (especially for maxByOrNull), and hard to understand difference. May be slightly rephrase/restructurize them.
Also it's hard to get confused about max()/maxFor { }/max { } , I'd emphasise differences/commons between them.
Please add links to pivot/groupBy grammars.
Make comments in examples code more detailed.
| * Link to it with `{@include [MaxDocsLink]}`. | ||
| * } | ||
| * | ||
| * ## The Max Operation |
There was a problem hiding this comment.
Please, remove all headers
Could you take a look, why Claude decided to bring header back? Because of examples or why?
There was a problem hiding this comment.
Sorry, now I’ve realised what this is topic, nevermind
There was a problem hiding this comment.
indeed, I checked the others were gone, I think it's fine to introduce this concept, right?
| * | ||
| * ## The Max Operation | ||
| * | ||
| * Computes the [maximum](https://en.wikipedia.org/wiki/Maximum_and_minimum) of values. |
There was a problem hiding this comment.
Do we need this link here actually?
There was a problem hiding this comment.
not necessarily, but it was on the website, so for completeness it might be nice. Just in case people are confusing maximal and maximum or something
| * `null` values in the input are always ignored. | ||
| * | ||
| * If the input contains [`NaN`][NaN] values, the result will be `NaN`, | ||
| * unless `skipNaN` is set to `true`. |
There was a problem hiding this comment.
can be linked to an argument
| * {@include [MaxDocs.ThrowsOnEmptySnippet]} | ||
| * | ||
| * See also: | ||
| * - [maxOrNull][DataColumn.maxOrNull] — returns `null` instead of throwing for an empty column. |
There was a problem hiding this comment.
or column of nulls
| maxOrNull(skipNaN).suggestIfNull("max") | ||
|
|
||
| /** | ||
| * Returns the maximum of the values in this [DataColumn], or `null` if there is nothing to compare. |
There was a problem hiding this comment.
I'd add
(i.e., column is empty or only `null` values are compared)
There was a problem hiding this comment.
Ok, there's a comprehensive snippet below, it's ok.
| * | ||
| * See also: | ||
| * - [maxByOrNull][DataColumn.maxByOrNull] — returns `null` instead of throwing for an empty column. | ||
| * - [maxOf][DataColumn.maxOf] — returns the maximum [selector] value itself |
There was a problem hiding this comment.
I'd place maxOf above see also section, with a message
"Don't confuse it with [maxOf] that returns maximum value computed with [selector] instead of the original element"
And the same in [maxOf] KDoc.
(and in maxByOrNull, min..()s, etc.)
| * {@include [SelectingColumns.ColumnGroupsAndNestedColumnsSnippet]} | ||
| * | ||
| * See [Selecting Columns][MaxDocs.MaxSelectingOptions]. | ||
| * |
There was a problem hiding this comment.
All link to pivot grammar
| * ### Example | ||
| * ```kotlin | ||
| * // The largest of all `Int` values in the first row | ||
| * // (so, in the "age" and "weight" columns), or `null` if there are none |
There was a problem hiding this comment.
(so, in the "age" and "weight" columns)
Add more context —
"Assuming there's two int columns in DF — age and weight,..."
| * | ||
| * ### Example | ||
| * ```kotlin | ||
| * // For each city, the largest value of each comparable column |
There was a problem hiding this comment.
We need to write a more detailed comment: "city" is a column; we group by it and take the maximum value from each group, and a new column called ‘max’ will appear with this value.
Fixes #1971, #1974
I let Claude attempt write it using all our KDoc guides. It's quite impressive!
Only made some small fixes and maintainability improvements, but it looks good to me. All questions I had are answered in the KDocs and it seemed to understand and use KoDEx quite well.