Skip to content

Conversation

@Allex-Nik
Copy link
Collaborator

Fixes #1497

@Allex-Nik
Copy link
Collaborator Author

The checks fail for now because in the documentation I refer to [DocumentationUrls.Pivot] which is added by @AndreiKingsley in the PR #1554 but not merged yet. Is it fine to have it in this state until #1554 is merged, or should I delete [DocumentationUrls.Pivot] for now and add it later?

*/
class LastTests : ColumnsSelectionDslTests() {

// region ColumnsSelectionDsl
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking that a test file could benefit from these regions as well. Is it a good idea to add it here?

* and returns a [ReducedGroupBy] containing these rows
* (one row per group, each row is the last row in its group).
*
* If the group in [GroupBy] is empty,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mentioned that it will return null for empty groups, but there is still the issue I mentioned for this case in #1547:
https://github.com/Kotlin/dataframe/pull/1547/files#r2505104735

Copy link
Collaborator

Choose a reason for hiding this comment

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

could you create an issue for that? I probably should have done that myself when finding the reproducer... But it makes it easier to track and refer to :)

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, sure! I probably should have done it right away, I just wasn't 100% sure :)

@Jolanrensen
Copy link
Collaborator

The checks fail for now because in the documentation I refer to [DocumentationUrls.Pivot] which is added by @AndreiKingsley in the PR #1554 but not merged yet. Is it fine to have it in this state until #1554 is merged, or should I delete [DocumentationUrls.Pivot] for now and add it later?

I expect it to be merged soon. If it takes too long, you can also copy the code from #1554 and let git handle the merge. Whatever you like best :)

/**
* Returns the last value in this [DataColumn].
*
* See also [lastOrNull], [first], [take], [takeLast].
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's also the @see tag if you just want to refer to other functions.

The only downside of @see is that you cannot add "extra" text to it. That's why we often write things like "See [something] for some reason."

However, if you don't need a description, you can simply write

@see [lastOrNull]
@see [first]

etc.

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.

Add unit tests and documentation for last.kt function

3 participants