Skip to content

Conversation

@etiennebacher
Copy link
Collaborator

Part of #1397

Copilot AI review requested due to automatic review settings November 12, 2025 22:12
Copilot finished reviewing on behalf of etiennebacher November 12, 2025 22:14
Copy link
Contributor

Copilot AI left a 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 PR implements the len() method for both GroupBy and LazyGroupBy classes to return the number of rows in each group, contributing to issue #1397.

Key Changes

  • Added <group_by>$len() and <lazy_group_by>$len() methods that return row counts per group
  • Included optional name parameter to customize the output column name (defaults to "len")
  • Comprehensive test coverage with both valid usage and error cases

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
R/lazyframe-group_by.R Implements lazygroupby__len() method with proper documentation
R/dataframe-group_by-general.R Implements groupby__len() method with documentation inheritance issue
man/lazygroupby__len.Rd Generated documentation for lazy group_by len method
man/groupby__len.Rd Generated documentation for group_by len method (contains incorrect title due to wrong inheritance)
tests/testthat/test-lazyframe-frame.R Adds comprehensive tests for both DataFrame and LazyFrame via expect_query_equal
tests/testthat/_snaps/lazyframe-frame.md Captures expected error messages for invalid parameter types
altdoc/mkdocs.yml Updates navigation to include new len methods in documentation
NEWS.md Documents the new feature in changelog

Copy link
Collaborator

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

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

Thank you.

I know this is a little late, but the methods for groupby and lazygroupby are exactly the same, so doing something like groupby__len <- lazygroupby__len seems sufficient.

@etiennebacher
Copy link
Collaborator Author

etiennebacher commented Nov 13, 2025

Ah yes indeed, it seems this is the case for many (all?) group_by functions. Is it fine if I do this change everywhere possible in a followup PR?

@eitsupi
Copy link
Collaborator

eitsupi commented Nov 13, 2025

That's good, thanks. I'll merge it for now.

@eitsupi eitsupi merged commit ef40791 into main Nov 13, 2025
21 checks passed
@eitsupi eitsupi deleted the groupby-len branch November 13, 2025 14:57
@eitsupi eitsupi added this to the 1.6.0 milestone Nov 13, 2025
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