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

feat: Add column_count, row_count and is_empty methods #119

Merged
merged 6 commits into from
Aug 21, 2023
Merged

feat: Add column_count, row_count and is_empty methods #119

merged 6 commits into from
Aug 21, 2023

Conversation

Techassi
Copy link
Contributor

@Techassi Techassi commented Jun 30, 2023

This PR adds three methods:

  • column_count: Returns the number of columns in the table
  • row_count: Returns the number of rows in the table
  • is_empty: Returns if the table is empty (contains no data rows)

@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2023

Codecov Report

Merging #119 (044cf03) into main (675125a) will decrease coverage by 0.16%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #119      +/-   ##
==========================================
- Coverage   96.52%   96.36%   -0.16%     
==========================================
  Files          19       18       -1     
  Lines        1754     1763       +9     
==========================================
+ Hits         1693     1699       +6     
- Misses         61       64       +3     
Files Changed Coverage Δ
src/table.rs 85.59% <100.00%> (+0.70%) ⬆️

... and 2 files with indirect coverage changes

@Nukesor
Copy link
Owner

Nukesor commented Jul 5, 2023

Thanks for your contribution!

I'm currently on holiday until next week and won't be able to review your PR until then :)

Copy link
Owner

@Nukesor Nukesor left a comment

Choose a reason for hiding this comment

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

I actually want to challenge the addition of the _count methods.

I'm not sure if this is particularily useful, since the alternative .row_iter().count() and .column_iter().count() really isn't that much longer and more complex.
I don't see the benefit of adding this yet.

Regarding the is_empty() call; I'm a bit more inclined to do this, since the alternative would be row_iter().next().is_none(), which isn't ergonomic at all.

But I want to ponder for a moment on what an empty table actually is?
Is it a table without rows?
What if there's a header?
What if there're rows, but the rows don't have any cells?

Personally I'm not sure which of these conditions should be met for a table to be considered empty.

src/table.rs Outdated Show resolved Hide resolved
@Techassi
Copy link
Contributor Author

I actually want to challenge the addition of the _count methods.

I'm not sure if this is particularily useful, since the alternative .row_iter().count() and .column_iter().count() really isn't that much longer and more complex.
I don't see the benefit of adding this yet.

The current implementation avoids constructing an iterator which is then immediately consumed by the .count() call.

But I want to ponder for a moment on what an empty table actually is?
Is it a table without rows?
What if there's a header?
What if there're rows, but the rows don't have any cells?

Yes I was thinking about the same stuff you did. Currently I consider the table to be empty if there are no data rows. This excludes the header. A header being there without any data rows is still considered empty.

We could expand this implementation and also check if the rows contain cells, and if they don't, the table will be empty.

@Techassi Techassi requested a review from Nukesor July 31, 2023 08:51
@Techassi
Copy link
Contributor Author

Techassi commented Aug 7, 2023

Any update on this? @Nukesor

@Nukesor
Copy link
Owner

Nukesor commented Aug 21, 2023

I guess these changes are fine :)

The argument of "a table is empty if no data rows are in it" seems reasonable as well.

Thanks for the contribution!

@Nukesor Nukesor merged commit aebf4ef into Nukesor:main Aug 21, 2023
@Techassi Techassi deleted the feature/row-col-count branch August 24, 2023 12:30
@Techassi Techassi changed the title feat: Add col_count, row_count and is_empty methods feat: Add column_count, row_count and is_empty methods Aug 24, 2023
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