Skip to content

Convert KDocs #1134

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

Merged
merged 15 commits into from
Apr 29, 2025
Merged

Convert KDocs #1134

merged 15 commits into from
Apr 29, 2025

Conversation

AndrewKis
Copy link
Contributor

@AndrewKis AndrewKis commented Apr 18, 2025

Closes #1019.
Closes #1150.

@Jolanrensen Jolanrensen self-requested a review April 22, 2025 11:01
* Converts the values in the specified [columns\] to a target type
* or using a custom converter, keeping their original names and positions within the [DataFrame].
*
* This function does not immediately convert the columns but instead select columns to convert and
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

selects*

* returns a [Convert],
* which serves as an intermediate step.
* The [Convert] object allows you to specify how the selected
* columns will be converted using methods such as
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you say "such as" I expect a handful of examples, not all of them.

* [toURL][Convert.toURL],
* [toIFrame][Convert.toIFrame],
* [toImg][Convert.toImg], and [toDataFrames][Convert.toDataFrames]
* that return a new [DataFrame] with updated columns.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be nice is a small list of available conversions, like how it is on the website. People can then decide to check the grammar for all the specific overloads or simply use to<Type>()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYM?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like here https://kotlin.github.io/dataframe/convert.html, just a list of supported types

* {@include [DslGrammarLink]}
* {@include [LineBreak]}
*
* **[`convert`][convert]**` { columnsSelector: `[`ColumnsSelector`][ColumnsSelector]` }`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the cases where you write "backtick, space, something..., space, backtick". It's a rule in markdown that single spaces like this are trimmed away: https://github.github.com/gfm/#code-spans. It doesn't happen in the current markdown renderer in intellij, but I think this is just a matter of time. It can be solved by writing 2 spaces instead of one, like ` { columnsSelector: `. It's not required for cases like ` }`, because it doesn't have a space at the beginning ánd end.

@@ -147,9 +499,27 @@ public fun AnyCol.convertTo(newType: KType): AnyCol =
else -> convertToTypeImpl(newType, null)
}

/**
* Converts values in this `String` column to the specified type [C].
*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm maybe we should link to parse? since this is just like parse but with a predefined return type for the values

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

public fun <T : Any> DataColumn<T?>.convertToBoolean(): DataColumn<Boolean?> = convertTo()

// region convert URL

/**
* Converts values in the [URL] columns previously selected with [convert] to the [IFRAME],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*an IFRAME

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check the others as well, like "the IMG" -> an IMG

@JvmName("convertToURLFromStringNullable")
public fun DataColumn<String?>.convertToURL(): DataColumn<URL?> = map { it?.let { URL(it) } }

/**
* Converts values in the [String] columns previously selected with [convert] to the [URL],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no "the" before "URL", either "to a URL" or "to URL"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check the others as well :)

* where the first element is used as the column name, and the remaining elements as values.
*
* For more information: {@include [DocumentationUrls.Convert]}
*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could use a visual example likely... But I'm not entirely sure how

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYM?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like:

containsColumns = false
[[a, b, c], [1, 2, 3], [4, 5, 6]] ->

containsColumns = true
[[a, 1, 4], [b, 2, 5], [c, 3, 6]] ->

a b c
1 2 3
4 5 6

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes it a bit easier to understand maybe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add an example to website doc

*
* By default, treats the first inner list as a header (column names), and the remaining lists as rows.
* If [containsColumns] is `true`, interprets each inner list as a column,
* where the first element is used as the column name, and the remaining elements as values.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could also do with a visual, but I'm not sure how

@AndrewKis AndrewKis requested a review from Jolanrensen April 22, 2025 15:04
# Conflicts:
#	core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/convert.kt
@AndrewKis
Copy link
Contributor Author

@Jolanrensen regarding supported types reference - for now I've added a link to convert page on website, but when we will change this page, I'll add a section "Supported types" and will refer specifically on it (like `link`#supported_types)

@AndrewKis
Copy link
Contributor Author

@Jolanrensen now it also closes #1150 - to was deleted in a favor to asColumn.


public fun <T, C> Convert<T, C>.to(columnConverter: DataFrame<T>.(DataColumn<C>) -> AnyBaseCol): DataFrame<T> =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably add a @Deprecated annotation here, it can be ERROR right away but we should steer people to convert.asColumn

* such as [toStr][Convert.toStr], [toDouble][Convert.toDouble], and many others.
*
* For the full list of supported types,
* refer to the [documentation website]({@include [DocumentationUrls.Url]}/convert.html).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add this urls in the file instead of here in the doc. Makes it easier to refactor if all urls are in 1 place

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in all places you use this btw

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the full list of supported types: {@include [DocumentationUrls.Convert]} should probably look fine

* `| `__`.`__[**`to`**][Convert.to]`(type: `[`KType`][KType]`)`
*
* {@include [Indent]}
* `| `__`.`__[**`to`**][Convert.to]` { columnConverter: `[`DataFrame`](DataFrame)`.(`[`DataColumn`](DataColumn)`) -> `[`AnyBaseColumn`](AnyBaseColumn)` }`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be removed now

*
* Use the following methods to perform the conversion:
* - [to(kType)][to]/[to`<Type`>()][to] – converts columns to a specific type.
* - [to { columnConverter }][to] - converts columns using column converter expression.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you removed this

* // Select columns with json values and convert it to decoded `String`.
* df.convert { valueJson }.with { Json.decode(it) }
* // Convert all `Int` columns to `Duration`, multiplying each value by the corresponding value from the "coeff" `Double` column before conversion
* df.convert { colsOf<Int>() }.with { baseValue -> (baseValue * coeff).toDuration(DurationUnit.MICROSECONDS) }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(baseValue * coeff).microseconds is also a valid Kotlinx datetime notation right? may look less daunting

@@ -55,7 +55,7 @@ public fun <T, C> ReplaceClause<T, C>.with(newColumns: List<AnyCol>): DataFrame<
// TODO: Issue #418: breaks if running on ColumnGroup and its child

/**
* For an alternative supported in the compiler plugin use [Convert.asColumn]
* For an alternative supported in the compiler plugin use [Convert.to]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to doesn't exist anymore

# Conflicts:
#	core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/parse.kt
@AndrewKis AndrewKis requested a review from Jolanrensen April 29, 2025 12:05
* * [LocalDateTime], [LocalDate], [LocalTime], [Instant] ( (kotlinx.datetime and [java.time]),
* * [URL], [IMG], [IFRAME].
*/
interface SupportedTypes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it :D

@AndrewKis AndrewKis merged commit 2b1d7c3 into master Apr 29, 2025
4 of 6 checks passed
@AndrewKis AndrewKis deleted the convert_kdocs branch April 29, 2025 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider changing convert { }.to { } operation Add KDocs for convert
3 participants