-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Columns Selection DSL KDocs and missing API overloads #331
Conversation
…sl-docs # Conflicts: # core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/ColumnsSelectionDsl.kt # core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/ColumnsSelectionDsl.kt
…ion dsl with docs (still need to add tests)
added all overloads for col, colGroup, and frameCol in dsl with docs
…, kproperties including tests for all
# Conflicts: # core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/ColumnsSelectionDsl.kt # core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/update.kt # core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/ColumnsSelectionDsl.kt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice c: Let's see how this works out.
core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/constructors.kt
Outdated
Show resolved
Hide resolved
core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/constructors.kt
Outdated
Show resolved
Hide resolved
...generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/ColumnsSelectionDsl.kt
Outdated
Show resolved
Hide resolved
...generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/ColumnsSelectionDsl.kt
Outdated
Show resolved
Hide resolved
...generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/ColumnsSelectionDsl.kt
Outdated
Show resolved
Hide resolved
} | ||
|
||
df.select { | ||
"name"["firstName", "lastName"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it still looks more like "pathOf" for me than slicing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pathOf would be "name"["firstName"]["lastName"]
. I actually think this is kinda neat. It allows you to select multiple columns way down in a path: "some"["path"]["to"]["multiple", "columns"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it takes a bit more effort to "parse" this expression and recognize the difference and to understand that the result is multiple columns.
Here the difference is more obvious and it's still quite compact
"some"["path"]["to"].cols("multiple", "columns")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah for sure :) But again for completeness, should we remove it?
pathOf("path", "to")["a", "b"]
doesn't look that wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? I don't know, let's check how it looks in completion. How discoverable is this API, how many options there are
Adds docs surrounding the Column Selection DSLs
Depends on #318