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 Table.remove_header() #121

Closed
wants to merge 1 commit into from
Closed

feat: add Table.remove_header() #121

wants to merge 1 commit into from

Conversation

ip1981
Copy link
Contributor

@ip1981 ip1981 commented Jul 27, 2023

About the motivation: to be able to modify a table created elsewhere. The Clone implementation might be helpful too, but it's orthogonal to this patch.

Signed-off-by: Igor Pashev <pashev.igor@gmail.com>
@Nukesor
Copy link
Owner

Nukesor commented Aug 4, 2023

It's generally not allowed to remove any rows, including the header right now.

The reason for this is, that there's internal logic executed each time a row or the header is added, which results in the calculation of columns for the whole table.

With this, you could add a header with 5 columns, rows with 3 columns, remove the header and still have a table with 5 columns even though only three are used.

If we wanted the user to allow removing the header or rows, this would need to be handled.

@codecov-commenter
Copy link

Codecov Report

Merging #121 (858e968) into main (7f5fbc9) will decrease coverage by 0.05%.
Report is 2 commits behind head on main.
The diff coverage is 0.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     #121      +/-   ##
==========================================
- Coverage   96.29%   96.24%   -0.05%     
==========================================
  Files          19       18       -1     
  Lines        1754     1758       +4     
==========================================
+ Hits         1689     1692       +3     
- Misses         65       66       +1     
Files Changed Coverage Δ
src/table.rs 83.66% <0.00%> (-1.22%) ⬇️

... and 2 files with indirect coverage changes

@Nukesor
Copy link
Owner

Nukesor commented Aug 6, 2023

This is actually quite tricky.

Imagine this scenario:

  • A user adds a row with 5 cells (header or normal row) and we have now 5 columns.
  • A user adds some constraints to all 5 columns.
  • The user now adds two rows with 3 cells
  • The user removes the header

Exit scenario 1.1 - Removal of unused columns:
- The user prints the table. And there'll to be 3 columns
- The user iterates over the columns and there're only 3 columns

Exit scenario 1.2 - Unused columns aren't removed:
- The user prints the table, but there're still 5 columns.
- The user iterates over all columns, but there're still 5 columns.


If the user would have now instead continued to use the api and would have done this:

  • The user adds a new row with 5 cells.

Exit scenario 2.1 - Removal of unused columns:
- The user prints the table. And there'll to be 5 columns, but the Constraints on column 4 and 5 are gone.

Exit scenario 2.2 - Unused columns aren't removed:
- The user prints the table, but there're still 5 columns with the correct constraints.


The problem is, that I designed columns to be implicitly populated and Constraints are to be added once some rows were populated.

The API design is minimalistic, convenient, but it's not designed for complex interactions, where users can remove cells or rows half-way through. It's rather designed to be used in a classic simple builder-pattern like way.

I think that we shouldn't try to add this function, since it will lead to weird implicit cases, no matter how we decide to implement it. The only proper way I see to work around this, would be to make the creation of columns explicit, which would be a huge breaking change and a completely different approach to column handling.

I guess this is something we could think about in the long term, but the API design should be well thought through.

@Nukesor
Copy link
Owner

Nukesor commented Aug 6, 2023

Closing for now, since this would need more work to function properly.

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