-
Notifications
You must be signed in to change notification settings - Fork 75
format
kdocs++
#1346
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
format
kdocs++
#1346
Conversation
…mpatibilities, but if we don't fix it before 1.0, we never will
d4d766e
to
97f86c8
Compare
…n the tests module. Updated references to `format`
7e839d6
to
03ab7b1
Compare
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.
Pull Request Overview
This pull request enhances the format operation for DataFrame rendering with comprehensive KDocs, breaking changes, and new functionality. The main purpose is to provide full documentation and API improvements for the format DSL before the 1.0 release.
- Adds comprehensive KDocs and grammar documentation for the
format
operation - Introduces a breaking change by renaming
FormatDSL
toFormatDsl
following project conventions - Adds new convenience methods
at()
andnotNull()
to the format operation, similar to the update operation
Reviewed Changes
Copilot reviewed 35 out of 47 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
tests/src/test/kotlin/org/jetbrains/kotlinx/dataframe/samples/api/Modify.kt | New test file with format operation examples demonstrating various formatting scenarios |
tests/build.gradle.kts | Updates Kandy dependencies and excludes dataframe dependency, adds format.md to documentation |
gradle/libs.versions.toml | Bumps Kandy version to include FormattedFrame support |
docs/StardustDocs/topics/format.md | New comprehensive documentation page for the format operation with examples and grammar |
docs/StardustDocs/topics/toHTML.md | Updates HTML rendering documentation to reference format operation |
docs/StardustDocs/topics/rendering.md | Updates rendering section to include format operation documentation |
Multiple HTML snippet files | Updates to HTML output files with additional CSS properties for improved text color handling |
Comments suppressed due to low confidence (1)
docs/StardustDocs/topics/format.md:3
- The TODO comment should be removed since the documentation has been completed in this PR.
<!---IMPORT org.jetbrains.kotlinx.dataframe.samples.api.Modify-->
a072b29
to
3c92afb
Compare
@@ -33,7 +33,7 @@ class FormattingTests : BaseTest() { | |||
|
|||
val formatter = formattedFrame.formatter!! | |||
for (row in 0 until typed.nrow) { | |||
FormattingDSL.formatter(typed[row], typed.age)!!.attributes().size shouldBe | |||
FormattingDsl.formatter(typed[row], typed.age)!!.attributes().size shouldBe |
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 add a deprecated type aliases for FormattingDSL and RGBColor?
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.
That could help with source-compatibility indeed :) Unfortunately binary compatibility will always break because we use FormattingDsl
as receiver in a lamda.
I'll add them
.format().with { bold and textColor(black) } | ||
.format("isHappy").with { background(if (it as Boolean) green else red) } | ||
.format("weight").notNull().with { linearBg(it as Int, 50 to blue, 90 to red) } | ||
.format("age").perRowCol { row, col -> |
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.
While we do have this function, using it is worrisome. We calculate min and max (which take full scan of the column) for each row, so it's n^2 complexity out of nowhere.
val col = df["age"] as DataColumn<Int>
val min = col.min()
val max = col.max()
.format("age").with {
textColor(
linear(value = it, from = min to blue, to = max to green)
)
}
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.
that... is a very good point. Let me see if I can adjust the examples to avoid repetitive calculations
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.
in practice it will probably not be that bad. I wonder how many people will try to format a table with hundreds of rows, but it's good to hint towards effecient behavior
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!
However, FormattedDF iframes don't work properly, but I will investigate that and fix in samples-utils.
@AndreiKingsley sure :) just bump the kandy version when you're done. I think I could already merge this version because the tables look good enough, right? |
Fixes #1280
Fixes #251 (I needed it in the example :) )
format
FormatDSL
toFormatDsl
, andRGBColor
toRgbColor
, following API names should follow CamelCase rules #509 before 1.0 is fully released, as it will be considerably more harmful to do later. Cannot go through deprecation cycle unfortunately.at()
, andnotNull()
toformat
, similar toupdate
, because they seem useful.where
filters to add to previous ones instead of overwriting the filter, similar toupdate
FormattedFrame
"sample dataframes" on the website#1356 and #1354 were discovered but can be done later.