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

Reimagine column handling #144

Open
Nukesor opened this issue Apr 6, 2024 · 1 comment
Open

Reimagine column handling #144

Nukesor opened this issue Apr 6, 2024 · 1 comment
Labels
f: help wanted Feel free to start working on this t: discussion Things that need a bit of a discussion t: enhancement Enhancing an existing feature or code

Comments

@Nukesor
Copy link
Owner

Nukesor commented Apr 6, 2024

The current column handling is rather convenient, as columns get populated as rows and cells are added to the table.

However, this logic is rather implicit. There's no way to remove or add columns half-way through a table creation process.

To fix this

  • column creation should be made explicit
  • or it should be possible to make column creation explicit

I'm actually in favor of the second approach, since the goal of comfy-table is to be minimalistic and convenient to use.
Explicit column handling is more of an edge-case and the 99% use case is covered by implicit column creation.

However, it could be very tricky to design the API around an optionally explicit column handling, since such APIs tend to get confusing quite quickly.

This ticket is mostly for discussion on how such an API would look like.

Relevant tickets/MRs:
#121
Previous ticket:
#124

@Nukesor
Copy link
Owner Author

Nukesor commented Apr 6, 2024

I gave this a lot of thought and here's what I came up with.

Current Problem

The current implementation doesn't allow:

  • Explicit addition of columns. Columns are always implicitely generated via rows.
  • Removal of rows. This isn't allowed as there's no explicit control over columns and Column removal isn't supported.
  • Columns to be swapped in position.

Design Proposal

Since this is about rethinking how the API would work and since this change will be breaking anyway, let's use this opportunity and do it properly.
My proposal for a new API design would look like this.

Column improvements

Accessing columns is currently annoying and error prone.
One needs to know the index of the column, which isn't ergonomic at all.
My proposal would be to move more logic into the column, which is why we need to make it more accessible.

  • The Column struct will gain a name in addition to its index.
  • The header will move from to the Column. This gives us more control over the header.
    -> Headers are removed from the Table.
    -> Column will get [set_]header functions.
  • Table will get a [set_]show_headers function to en/disable displaying of column headers.
  • Table will get a get_column_by_name() getter, which may return columns by their identifier.

Adding columns

  • There'll be a table.add_column(name: &str) -> Result<&mut Column> and a table.add_columns(columns: Vec<String>) -> HashMap<String, &mut Column> function.
  • There'll still be no way to add actual Column instances to the table.
    Having a &mut Column returned should make things easier though.

Removing columns

I found no good way of actually removing columns.
The problem with column removal is, that it needs to implicitely remove the respective cells of all current rows as well.
Without that, we wouldn't know which cells to display or not.

As the whole idea of this request is to remove implicit logic, I decided to forbid removal of columns.
Instead of removing columns, it should be made more convenient to hide them, which is basically the same as a temporary "soft" removal.

Hiding columns

Currently, the functionality to hide a column is implemented as a Constraint, which is a bit dirty.
It's not necessarily a constraint and having it as a constraint toggling the column in a program annoying, since the old constraint needs to be restored as soon as the column is made visible again.

  • Add a hidden field to the Column in addition to respective hide(bool)/hidden() getter/setter functions.
  • Remove the ColumnConstraint::Hidden variant.

Error handling

Previously, comfy-table was able to do things in an "optimistic" manner.
It was just given some strings and from there on it tried to display things to the best of its abilities.

With the trend to giving developers more power, we also have to introduce error handling.

  • Add a ComfyError class, depending on the needed complexity introduce thiserror.
  • All setters that might fail due to missing names/duplicate identifiers need to return Result<T, ComfyError>.
  • There should be appropriately named error variants based on the type of error, e.g. DuplicateColumn.
    All functions need to document which errors might be returned.

Requested Feature: Removing rows/swapping columns

There were two additional requested features:

  • Remove rows
  • Swap columns

With the current builder design, these features won't be implemented, as this would only lead to confusion.
It would allow scenarios like this:

- Add some rows
- Swap some column positions
- Add some new rows in the original layout
-> The columns of the new rows are now in the wrong order.

There're two solutions to this problem:

  1. Make rows named as well.

    • We'll no longer use Vecs and only HashMaps instead.

    This solution makes the whole library harder to use for a very small userbase.

  2. Actually use an builder pattern.

    • Use a TableBuilder in which one may specify columns, rows and their content.
    • call builder.build(), which returns a Table.
    • That Table now only allows styling related actions, content may no longer be added/removed etc.

    This solution doesn't really solve the problem, as people also want to add content to the table after they've already built it once.

To be honest, I'm inclined to tell people that they should just rebuild the whole table if they want complex interactions.
Comfy-table is designed to be a minimalistic table builder library and shouldn't be used as an interactive table renderer.

-> Until there's a good design proposal that doesn't make the library less usable/unusable for the vast majority of its users, these two features won't be added.

Who builds this?

I am very happy with the current state of comfy-table and I don't need any of said functionality myself.
Even though I designed and specified the new architecture, I won't implement it.
Also, if you find any design flaws, please point them out and let's try find a better solution.

As usual, I'll give any MRs thorough reviews as long as they're well documented and properly tested (via actual tests).

So, if anyone needs this and has some spare time at their hand, I would be happy to see this merged into comfy-table :).

@Nukesor Nukesor added t: enhancement Enhancing an existing feature or code f: help wanted Feel free to start working on this t: discussion Things that need a bit of a discussion labels Apr 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: help wanted Feel free to start working on this t: discussion Things that need a bit of a discussion t: enhancement Enhancing an existing feature or code
Projects
None yet
Development

No branches or pull requests

1 participant