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

Change formatting to two pass so that columns can be aligned and provide text wrapping to tty column size. #549

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

elric1
Copy link

@elric1 elric1 commented Feb 29, 2024

No description provided.

@elric1 elric1 force-pushed the formatting branch 5 times, most recently from f4f7b54 to 36f0422 Compare March 3, 2024 10:09
@WhyNotHugo
Copy link
Member

We used to have table rendering, but it was removed in 586c0c2.

Maybe we can bring it back behind a config option (e.g.: make it optional). I think the Formatter plumbing might work for this.

@elric1
Copy link
Author

elric1 commented Mar 3, 2024

Oh, I didn't see that. It does end up being a little different than what I was doing, in a number of ways:

  1. the [X] for completed was flipped in 586c0c2 and I preserved that not knowing that it was flipped.
  2. I'm wrapping the summary/categories column. Tabulate can't wrap in the way that I'm doing as it requires you to specify a maxcolwidth for each column, but can't do it for the whole screen. This is spoken of in Tablulate Issue #290, but the proposed solution there is also suboptimal as it would merely set the maxcolwidth to tty cols / num cols which would leave the summary column way too small.
  3. I'm entirely leaving out columns that don't exist which makes it a bit more compact. I could still do this with tabulate, though, but it would still be a two pass algorithm.

Is using tabulate important for this patch, or is it acceptable to just format it ourselves? Tabulate appears to provide quite a few extra types of output which could be quite nice to offer such as HTML or LaTeX, so maybe the best strategy would be to extend tabulate and submit a PR with them and use it?

@elric1
Copy link
Author

elric1 commented Mar 4, 2024

In accordance with your comments above, I have changed approach. Now, I submit a patch to use tabulate, but I make it configurable so that the TableFormat can be selected by the user. They can therefore use plain, html, markdown, etc. I also submitted a pull request to the tabulate project to add a "flow" table type which does not align the columns and drops empty columns: Tabulate PR#314

@WhyNotHugo
Copy link
Member

Tests indicate that there are extra spaces in the default outputs: https://builds.sr.ht/~whynothugo/job/1161850#task-test-517

@WhyNotHugo
Copy link
Member

You can run tests locally with tox -e py.

@WhyNotHugo
Copy link
Member

Feel free to ask if you have issues running tests.

@WhyNotHugo
Copy link
Member

This approach is good, using tabulate is a lot less code and I know it works well already 👍

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.

2 participants