ColumnsSelection DSL overhaul #372
Conversation
Jolanrensen
added
enhancement
…-type-safe apis to access ColumnGroups. Refactoring
…olumn no longer implements ColumnSet. This means proper separation per function in the selection DSL. Requires sometimes some explicit conversion to ColumnGroups.
…Last)(Children)While
… applicable. Expanded KProperty examples, started with filter {}
…ntains, nameStartsWith, nameEndsWith
… to avoid clashes with stdlib functions for string and lists
|
It's interesting, could we split the model/plugin changes from the documentation changes? |
|
Some small improvements that need to be made before merging this PR:
|
…rity, specifying at which level they operate, mentioning scoping for except and improving examples.
# Conflicts: # docs/StardustDocs/snippets/org.jetbrains.kotlinx.dataframe.samples.api.Access.columnSelectors.html # docs/StardustDocs/snippets/org.jetbrains.kotlinx.dataframe.samples.api.Access.columnSelectorsMisc.html # docs/StardustDocs/snippets/org.jetbrains.kotlinx.dataframe.samples.api.Access.columnSelectorsModifySet.html # docs/StardustDocs/snippets/org.jetbrains.kotlinx.dataframe.samples.api.Access.columnSelectorsUsages.html # docs/StardustDocs/snippets/org.jetbrains.kotlinx.dataframe.samples.api.Modify.convert.html # docs/StardustDocs/snippets/org.jetbrains.kotlinx.dataframe.samples.api.Modify.update.html
Jolanrensen
requested review from
zaleslaw and
koperagen
and removed request for
koperagen
core/build.gradle.kts
Outdated
| @@ -205,6 +205,7 @@ val processKDocsMain by creatingProcessDocTask( | |||
| ARG_DOC_PROCESSOR, | |||
| COMMENT_DOC_PROCESSOR, | |||
| SAMPLE_DOC_PROCESSOR, | |||
| // REMOVE_ESCAPE_CHARS_PROCESSOR, | |||
There was a problem hiding this comment.
will be in the next release of the doc preprocessor, I must not forget to enable it then :)
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/colsAtAnyDepth.kt
Show resolved
Hide resolved
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/colsAtAnyDepth.kt
Outdated
Show resolved
Hide resolved
| * If no column adheres to the given [condition\], [NoSuchElementException] is thrown. | ||
| * | ||
| * This function only looks at columns at the top-level. |
There was a problem hiding this comment.
looks at columns - is unclear for me (search among?)
There was a problem hiding this comment.
replaced with "operates"
| * {@setArg [UpdateOperationArg] [**update**][update]}{@comment The default name of the `update` operation function name.} | ||
| */ | ||
| public interface Usage | ||
| public interface Grammar |
There was a problem hiding this comment.
GrammarDI or GrammarDocInterface. I'm starting to think, that if we could add prefix or postfix to filter easier all the documentation interfaces
There was a problem hiding this comment.
hmm, I don't know, as you can also copy docs from other functions or classes, which are not solely "documentation interfaces" like these and those won't have a special name. Plus, this name is also what people will see when they read the KDocs:
Now it just says "public interface Grammar", which looks nice, plus I can also refer to it without alias, like See [Grammar] for....
What we could do, maybe in the future, is to have two main interfaces somewhere: PublicDocInterface, which all documentation interfaces that are user-facing implement, and InternalDocInterFace or TempDocInterface which would indicate which doc interfaces would be removed after the docs are processed. That might clean up all intermediate doc interfaces neatly. (The doc processor cannot do this (yet) btw, just an idea).
There was a problem hiding this comment.
Actually, that's where @Annotations would make more sense I think, like:
/**
* Some Doc
*/
@ExcludeFromSources
internal interface Doc
Discussed in #396
Originally posted by Jolanrensen June 9, 2023
Hi everyone!
While working on the Columns Selection DSL, which is an integral part of the library, I found many inconsistencies and took it upon myself to fix that. I documented almost every function and reworked all overloads, but I still have doubts about some.
So, I collected these concerns in a helpful Kotlin Notebook for you to experience and judge! The notebook imports a proposal version of DataFrame based on the extensive PR, so you can experience the changes firsthand.
The data used for the example can be found here and to view and run the notebook, I recommend the Kotlin Notebook Plugin for IntelliJ.
You can also use Datalore or the Kotlin Jupyter kernel, but then you'd miss out on the new and extensive KDocs, so I wouldn't recommend that.
Please leave your feedback below in the corresponding thread below. I'm curious to hear your feedback! Also, feel free to leave any questions below as well if my notebook isn't clear enough in explaining the issues.
For reviews, I'll break it up into parts. I'll mark items that can be reviewed in bold and with corresponding files (or sections). When the reviewing of a certain section is done, I'll check the box in front of it. Hopefully, that way we can check off all the functions in an ordered manner :). To check out the docs I'd recommend checking out the branch, publish to maven local, and use it in a sample project (or notebook). Then you can see how they are rendered in IntelliJ and test out the links.
New ColumnsSelectionDsl/ColumnSet/SingleColumn split
First
Last
Single
Constructors
asColumnGroup()functions here and expanded + with kdocsCol
ValueCol (new)
FrameCol
ColGroup
Cols
Column range
ValueCols
cols { predicate }but only of kind ValueColumn.ColGroups
cols { predicate }but only of kind ColumnGroup.FrameCols
cols { predicate }but only of kind FrameColumn.ColsOfKind
cols { predicate }but only of given kinds. Extra useful in combination with recursively/atAnyDepthAll
Colsinfix. Fixed behavior using testColumnSet.all(Before/ etc)(column)?allColumnsInternalfunction is the new internalatAnyDepth-compatible function that's used anywhere the "children" of a ColumnsResolver need to be taken. It keeps the original behavior of the library based on all tests and examples.Recursively/ ColsAtAnyDepthChildren/ ColsInGroupscolsInGroupsand fixed the behavior. The function now always operates on all groups inside the column set, or on all groups inside the SingleColumn/ColumnGroup.Take
Drop
ColumnNameFilters
ignoreCaseargument.WithoutNulls
Distinct
None
ColsOf
Roots/SimplifyFilter
And
Rename
Select
invoke {}overload and other missing overloads.AllExcept
exceptNewto become a potential replacement forcolGroup except { someCol }Expr
documentation for the website