Prettytable is unmaintained. Change to comfy-table

[PsiACE]

Change to comfy-table from prettytable. In addition, comfy-table is still actively maintained.

Signed-off-by: Chojan Shang [email protected]

[codecov]

Codecov Report

Merging #251 (0457a0d) into main (d545253) will increase coverage by 0.00%.
The diff coverage is 75.00%.

@@           Coverage Diff           @@
##             main     #251   +/-   ##
=======================================
  Coverage   76.76%   76.76%           
=======================================
  Files         232      232           
  Lines       19820    19820           
=======================================
+ Hits        15214    15215    +1     
+ Misses       4606     4605    -1     

Impacted Files	Coverage Δ
src/io/print.rs	96.85% <75.00%> (ø)
src/io/csv/write/mod.rs	88.88% <0.00%> (+1.38%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d545253...0457a0d. Read the comment docs.

[jorgecarleitao]

Awesome! Thanks a lot, @PsiACE !

[jorgecarleitao]

fwiw, I did go through the code for unsafe; Nukesor/comfy-table#42 (not to its dependencies yet)

[Nukesor]

strum and strum_macros don't have unsafe in their code-base.
crossterm has a few unsafe blocks, which are necessary for terminal control.

comfy_table actually does use one of those functions to check, if the current program is on a TTY or not.
It should be this block in crossterm:

src/tty.rs
33:        unsafe { libc::isatty(fd) == 1 }

In the current code, it get's always called if there is no explicit width set via Table::set_table_width().
It isn't in your case, so this is relevant.

I think this could be refactored to only be called if either:

• Percental Constrains exist. as we need to know the current width for percental calculations.
• The user uses Dynamic mode, as this mode takes all content and formats it in a way that it fits into the given space.

Dynamic content arrangement in ttys is the core feature of comfy-table, which makes this libc call a necessary evil.
As this isn't a thing in Prettytable, it seems to be safe in this regard.

[Nukesor]

Turns out, one more call is used. Crossterm's size function, which is used to determine the terminal size.
This function is only called in conjunction with is_tty, if a tty has been detected.

    if wrap_with_result(unsafe { ioctl(fd, TIOCGWINSZ.into(), &mut size) }).is_ok() {

This seems to be another libc call with, which his used to communicate with /dev/tty via a raw filedescriptor.
http://rosettacode.org/wiki/Terminal_control/Dimensions#Library:_BSD_libc

All unsafe functions are called from comfy-table's Table::get_table_width:

    /// Get the expected width of the table.
    ///
    /// This will be `Some(width)`, if the terminal width can be detected or if the table width is set via [set_table_width](Table::set_table_width).
    ///
    /// If neither is not possible, `None` will be returned.\
    /// This implies that both the [Dynamic](ContentArrangement::Dynamic) mode and the [Percentage](crate::style::ColumnConstraint::Percentage) constraint won't work.
    pub fn get_table_width(&self) -> Option<u16> {
        if let Some(width) = self.table_width {
            Some(width)
        } else if self.is_tty() {
            if let Ok((table_width, _)) = size() {
                Some(table_width)
            } else {
                None
            }
        } else {
            None
        }
    }

[Nukesor]

Turns out, I already had this issue once and you can explicitly disable the whole tty logic.
Just call Table::force_no_tty and you're good to go.

I'm also open to a PR, which hides this behind a feature flag. (Tty should still stay the default though, as it's still comfy-table's core feature)

[jorgecarleitao]

I agree that feature flagging this upstream would be useful to this use-case since we do not care about terminals (that is e.g. nutshell's business, cc @elferherrera :)

Regardless, being maintained is imo more relevant than these :)