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

Added 3 APIs for Flatten function #307

Merged
merged 2 commits into from
Mar 17, 2023
Merged

Added 3 APIs for Flatten function #307

merged 2 commits into from
Mar 17, 2023

Conversation

zaleslaw
Copy link
Collaborator

Fixed #292

@zaleslaw
Copy link
Collaborator Author

@koperagen @Jolanrensen I have a question: why do we miss KProperties as an Access API in the documentation (I could not find tabs with that kind of access). Is it related that it requires a plugin and it is somehow "difficult" for a user?

I also suggest updating the documentation in PRs together with the API changes as a part of triad (code, tests, docs)

@Jolanrensen
Copy link
Collaborator

@koperagen @Jolanrensen I have a question: why do we miss KProperties as an Access API in the documentation (I could not find tabs with that kind of access). Is it related that it requires a plugin and it is somehow "difficult" for a user?

I also suggest updating the documentation in PRs together with the API changes as a part of triad (code, tests, docs)

Like here? https://kotlin.github.io/dataframe/kpropertiesapi.html

@zaleslaw
Copy link
Collaborator Author

Not, probably I wrote unclear. We have 4 apis but on many pages in operations we have 3 tabs only without Kproperties

@@ -11,13 +11,36 @@ flatten [ { columns } ]
Columns after flattening will keep their original names. Potential column name clashes are resolved by adding minimal possible name prefix from ancestor columns.

<!---FUN flatten-->
Copy link
Collaborator

Choose a reason for hiding this comment

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

you renamed this function to flatten_properties in Modify.kt

We use Korro to check that samples in stardust are actually compilable. So make sure that all examples in markdown are surrounded like
<!---FUN someFunctionName-->
your example, which is between // SampleStart and // SampleEnd comments in samples/api/Modify.kt inside a function with the same name.
<!---END-->

Copy link
Collaborator Author

@zaleslaw zaleslaw Mar 17, 2023

Choose a reason for hiding this comment

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

Look, for example insert

It has in documentation the following


<!---FUN insert-->
<tabs>
<tab title="Properties">

and in modify the following


  @Test
    fun insert_properties() {
        // SampleStart
        df.insert("year of birth") { 2021 - age }.after { age }
        // SampleEnd
    }

    @Test
    fun insert_accessors() {
        // SampleStart
        val year = column<Int>("year of birth")
        val age by column<Int>()

        df.insert(year) { 2021 - age }.after { age }
        // SampleEnd
    }

    @Test
    fun insert_strings() {
        // SampleStart
        df.insert("year of birth") { 2021 - "age"<Int>() }.after("age")
        // SampleEnd
    }

Looks like I do the same, but is the best way to check that I am right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Template looks like: name of function underscore acess api

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh now I see! Didn't know this was possible. Cool :)

@Jolanrensen
Copy link
Collaborator

Not, probably I wrote unclear. We have 4 apis but on many pages in operations we have 3 tabs only without Kproperties

I see, probably that is because it was added later. Should definitely be the same everywhere :)

@koperagen
Copy link
Collaborator

Not, probably I wrote unclear. We have 4 apis but on many pages in operations we have 3 tabs only without Kproperties

I think it was done deliberately, but might be discussed. There's not much value in using this API outside its rather narrow use case, like described here https://kotlin.github.io/dataframe/kpropertiesapi.html. So using only 3 APIs in samples was probably an attempt to find a balance between providing this feature for advanced use cases and not confusing people as to what to use.

@zaleslaw
Copy link
Collaborator Author

@Jolanrensen fixed the review

@zaleslaw zaleslaw merged commit a00602b into master Mar 17, 2023
@Jolanrensen Jolanrensen deleted the zaleslaw/issue-292 branch March 17, 2023 13:11
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.

flatten lacks overloads
3 participants