Skip to content
This repository has been archived by the owner on May 13, 2022. It is now read-only.

format tables nicely fixed-width with headers+borders via texttable.py. #539

Open
wants to merge 51 commits into
base: master
Choose a base branch
from

Conversation

dan-da
Copy link

@dan-da dan-da commented May 26, 2016

Hi, I found the output of wallet-tool.py a bit hard to read especially in the history report because the column data was not matching up with col headers and also text was wrapping in my shell.

So I made this patch that formats tabular data nicely into fixed-width text tables with borders. I'm quite pleased with the result, at least for my purposes. Here's a simple example:

+-----------+---------------+
| Mix Depth | Balance       |
+-----------+---------------+
|         0 | 0.34898343btc |
|         1 | 0.23468934btc |
|         2 | 0.23123434btc |
|         3 | 0.82397493btc |
|         4 | 0.22356223btc |
+-----------+---------------+
| total:    | 1.84839232btc |
+-----------+---------------+

(note: above numbers are fictitious.)

Column widths are auto-calculated based on longest value and numeric values are right-justified.

CSV output of the history report still works. I changed it to use csv.writer().

It is important to mention about what happens when table width exceeds terminal width. I'll just paste the code comment about it.

# a function to print a text table.
# Detects if running in a TTY and if so disables terminal wrapping
# because the wrapping is too ugly for words.
# In this mode, terminals truncate text if line is too long.
# A good way to view the full table if it is too wide for terminal
# is to pipe output through less -S, which enables horizontal scrolling.
#   eg:  python wallet-tool.py <args> | less -S

(Another way could be to redirect to a file, then view with less -S or any text editor)

If the patch is accepted, it would be advisable to mention this detail in the help and readme.

…n/additions to sendpayment, taker, wallet code to accommodate
…le help text, used this for donation help text, set donation off by default
…tart, which is a little inelegant workflow but a much easier code patch
…in section GUI, and persist config to joinmarket.cfg on update, a couple more minor tidy up edits
@chris-belcher
Copy link
Collaborator

I don't think this should be merged (sorry.) I've always believed the CLI outputs should be as simple as possible without too many bells and whistles. I think it might be more appropriate to add things like this to a GUI interface.

@dan-da
Copy link
Author

dan-da commented May 29, 2016

Would you consider accepting the patch if I make it a non-default flag/option?

For me the text mode history report was pretty much unreadable before the patch with column data mismatched from headers and line wrapping.

@raedah
Copy link
Contributor

raedah commented May 30, 2016

@dan-da Why is it that your terminal needs line wrapping with this? Do you have a very narrow terminal?

@dan-da
Copy link
Author

dan-da commented May 30, 2016

@raedah For me the tabular data line wrapping occurs with the 'history' command (which has many columns), not the other commands.

I have a wide screen (for a laptop) and have the shell full-width but I use a larger-than-average font for less eye-strain.

Also, many people in unix environment use shells that are not full screen width.

This patch provides output that is pretty much identical to mysql's result format, or also postgresql. I would argue that it is a common method of displaying tabular data in text mode environment, and one that people are familiar with.

@chris-belcher
Copy link
Collaborator

@dan-da could you output it as csv and then view with spreadsheet software? (even something like sc, CLI spreadsheet app)

@dan-da
Copy link
Author

dan-da commented May 31, 2016

@chris-belcher It's on a server, hence text mode. copying csv output to local machine each time would be a huge hassle compared to eg passing --texttable flag to wallet-tool.py. There's a reason that mysql, postgres, etc format tabular data this way...

@chris-belcher
Copy link
Collaborator

Are you able to install sc or another text spreadsheet program on your server ?

@dan-da
Copy link
Author

dan-da commented May 31, 2016

hadn't heard of sc. interesting idea. still the csv is intermixed with non csv output. I'm not sure how that would format inside sc or if it would even read it correctly without first extracting the csv. maybe I'll give it a try for kicks when server is back up/running.

still, for my purposes it is just as easy to maintain a patched fork, which is what I'll probably do.

Quit frankly I think y'all have an "interesting" way of treating new contributors to the project. I saw you didn't want the new behavior by default so I offered to modify the patch to keep the old behavior and only add the new as a flag, so only those who WANT it will ever see it. This is is a simple patch, one that doesn't touch any core logic or behavior and in my opinion enhances usability, at least for some. Instead of taking me up on that offer you suggest kludgy workarounds that I could come up with on my own... if I wanted kludgy workarounds.

whatever, I get it. It is your project and your vision. your call. And I thank you for the work you've put into joinmarket for the community! Privacy and fungibility are super important and I applaud all efforts towards strengthening those in the ecosystem. After this though I doubt I'll be investing my time in further patches.

feel free to close this.

@raedah
Copy link
Contributor

raedah commented May 31, 2016

Just part of the territory, practicing working and discussing as a team. So far this patch applies to a very limited situation of only on the 'history' cmd, and with a large font. So I think its valid to consider the extra code maintenance it may bring. A lot of patches are often maintained separately, and then a later more refined version might get merged. No rush. We appreciate your contribution and effort.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants