Move toLeft -> toStart, toRight -> toEnd, add move KDocs #1071
Conversation
|
Uhm, we should probably keep the old functions, deprecate them (with ERROR right away is fine), and direct our users to the new ones. |
|
Brought them back with Error-level deprecation |
| * {@include [LineBreak]} | ||
| * {@include [DslGrammarLink]} | ||
| * {@include [LineBreak]} | ||
| * |
There was a problem hiding this comment.
Very nice! However, you're missing the definitions of the types you use. (fuck, I see I forgot it in update as well) Each grammar dsl in the selection dsl does have definitions on top. something like:
`colsSelector: `[ColumnsSelector]
`pathSelector: `[...]
This makes the grammar complete if we want to reuse it on writerside with @ExportAsHtml (Since there you cannot click on types anymore).
In the selection dsl, for each reference, I made a -Def and a -Ref variant (https://github.com/Kotlin/dataframe/blob/master/KDOC_PREPROCESSING.md#advanced-dsl-grammar-templating-columns-selection-dsl), something like:
/** `colsSelector: `[`ColumnsSelector`][ColumnsSelector] */
public interface ColumnsSelectorDef(included at the definitions part of the grammar dsl)
/** [`colsSelector`][ColumnsSelectorDef] */
public interface ColumnsSelectorRef(included at the actual notation part of the grammar dsl)
That way users can click on the reference, see the definition, and in the definition click on the type to see the actual type. This way you can also have a reference to a definition with multiple types allowed.
| /** | ||
| * ## The Move Operation | ||
| * | ||
| * Moves the specified [columns] within the [DataFrame]. |
There was a problem hiding this comment.
KoDEx incorrectly recognizes [columns] as org.jetbrains.kotlinx.dataframe.columns, making it link up wrong. You can break the reference here by writing [columns\]
There was a problem hiding this comment.
Also check this in other places as well
| * which serves as an intermediate step. The [MoveClause] allows specifying the final | ||
| * destination of the selected columns using methods such as [MoveClause.to], [MoveClause.toStart], | ||
| * [MoveClause.toEnd], [MoveClause.into], [MoveClause.intoIndexed], [MoveClause.toTop], | ||
| * [MoveClause.after] or [MoveClause.under], that return a new [DataFrame] with updated columns order. |
There was a problem hiding this comment.
probably alias these links like [to][MoveClause.to], that reads a bit easier :)
There was a problem hiding this comment.
It's not just an updated order, right? It's an updated structure, I'd say
| * @include [CommonMoveDocs] | ||
| * @include [SelectingColumns.Dsl] {@include [SetMoveOperationArg]} | ||
| * ### Examples: | ||
| * ``` |
There was a problem hiding this comment.
I'd write these examples starting with "```kt" so they are colored like Kotlin code
| * df.move("columnA", "columnB").after("columnC") | ||
| * df.move("age").under("info") | ||
| * ``` | ||
| * @param [columns] The [Column Names][String] used to select the columns of this [DataFrame] to move. |
There was a problem hiding this comment.
If you want to link to something for column links, could I suggest org.jetbrains.kotlinx.dataframe.documentation.SelectingColumns.ColumnNames?
There was a problem hiding this comment.
I just followed a template from select KDocs.
There was a problem hiding this comment.
* @param [columns] The [Column Names][String] used to select the columns of this [DataFrame].
*/
public fun <T> DataFrame<T>.select(vararg columns: String): DataFrame<T> = select { columns.toColumnSet() }
| /** | ||
| * ## The MoveToStart Operation | ||
| * | ||
| * Moves the specified [columns] to the [DataFrame] start. |
There was a problem hiding this comment.
maybe specify that this means the top-level, or do you think that's clear enough from this?
| * | ||
| * For more information: {@include [DocumentationUrls.Move]} | ||
| */ | ||
| internal interface MoveToEnd { |
There was a problem hiding this comment.
I probably couldn't help myself and create a generic MoveToX doc hahaha, but that would probably be overkill, great job :)
| /** | ||
| * ## Move Into | ||
| * | ||
| * Moves columns, previously selected with [move] into a new position specified with a |
| public fun <T, C> MoveClause<T, C>.to(columnIndex: Int): DataFrame<T> = moveTo(columnIndex) | ||
|
|
||
| /** | ||
| * ## Move ToTop |
There was a problem hiding this comment.
Seems you are a bit inconsistent with the names of the sub-operations. Some are written like "Move ToTop", others like "MoveToStart". I assume you did this to mirror the function names, however it may be more readable to just separate them all by spaces and use Titlecase, like "Move to Top", "Move to Start", etc. Also, you'll make nikita happy if you use a couple more #'s so the titles become a bit smaller
There was a problem hiding this comment.
OK, so let's make a convention for all of this.
My logic was like this:
If its a complete operation (that is DF-> DF : moveToStart, moveToEnd) then it will have the full name in Upper Camel Case.
If it's a part of move operation (MoveClase -> DF), I write "Move" + this operation in UCC.
There was a problem hiding this comment.
I would favor readability over it following the function name exactly here. Users can already see the function name when they click to read the KDoc, so the title can just be a simple readable title I think.
| * | ||
| * ### This After Overload | ||
| */ | ||
| internal interface MoveAfter |
There was a problem hiding this comment.
This can be @ExcludeFromSources, right?
There was a problem hiding this comment.
Probable :) Done.
There was a problem hiding this comment.
No objection from my side after fixing comments from @Jolanrensen
Great job, I've learned also something new about the KoDex
toLeft->toStart,toRight->toEnd#1027move#813