Skip to content
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

Korro outputs #370

Merged
merged 4 commits into from
May 5, 2023
Merged

Korro outputs #370

merged 4 commits into from
May 5, 2023

Conversation

koperagen
Copy link
Collaborator

@koperagen koperagen commented May 3, 2023

Now running korro task will not only update code snippets, but also copy-paste content of the file with name corresponding to FQ name of the korro FUN directive. So, it means that we can append arbitrary content to the docs by simply writing it into the file during test execution. korro will keep docs in sync with it.

Copy link
Collaborator

@zaleslaw zaleslaw left a comment

Choose a reason for hiding this comment

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

Unclear in a few moments for me, but the result of PR is great with the removal of all these links

df.select { it[Person::name][Name::firstName] }
df.select { Person::name[Name::firstName] }
df.select { it["name"]["firstName"] }
df.select { "name"["firstName"] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it really works this way, call [] on String? I don't know about it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is! It's an overloaded "get" operator.


// column arithmetics
df.select { 2021 - (Person::age)() }
df.select { 2021 - "age"<Int>() }
Copy link
Collaborator

Choose a reason for hiding this comment

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

honestly need to say, that Person::age was better than "age"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The PR is a bit unclear in this part. In fact, it's a very bad "diff". In this file after update of korro KProperties API tab was removed (we discussed it with Jolan). And "String API" tab was already there

Copy link
Collaborator

Choose a reason for hiding this comment

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

There has also been some shifting around, that's where the clashes come from


// depth-first-search traversal of all children columns
df.select { Person::name.allDfs() }
df.select { "name".allDfs() }
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like you and @Jolanrensen will have a clash here

@@ -38,6 +38,7 @@ val df3 = df.sortByDesc { age }
listOf(df1, df2, df3).fold(DataFrameHtmlData.tableDefinitions()) { acc, df -> acc + df.toHTML() }
```

<dataFrame src="org.jetbrains.kotlinx.dataframe.samples.api.Render.composeTables.html"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this line here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's how we embed the table component into the documentation now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But you're right, this specific example is not very useful, i'll remove it for now

@Jolanrensen
Copy link
Collaborator

Could you run assemble again to regenerate the docs before each commit?

@koperagen
Copy link
Collaborator Author

koperagen commented May 4, 2023

Could you run assemble again to regenerate the docs before each commit?

I updated Render.kt in generated sources. Is it what you asked for?

@@ -2,6 +2,6 @@ projectName=dataframe
version=0.11.0
jupyterApiTCRepo=
kotlin.jupyter.add.scanner=false
org.gradle.jvmargs=-Xmx2G
org.gradle.jvmargs=-Xmx4G
Copy link
Collaborator

Choose a reason for hiding this comment

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

back to 4 hey?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, korro needs it, otherwise fails with OOM

Copy link
Collaborator

Choose a reason for hiding this comment

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

nice

@koperagen koperagen merged commit 858911e into master May 5, 2023
@koperagen koperagen deleted the korro-outputs branch August 26, 2024 13:48
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.

3 participants