API changes #27

tertsdiepraam · 2023-10-31T15:43:21Z

Closes #25
Closes #26

Ultimately, I want to refine the API even more, but I think this is a pretty good step in the right direction. Note how all the tests and examples are now more declarative. The grid values never have to be mutable anymore.

There is a bit of an ugly thing, which is that dimensions is set to some unused dummy value before it is modified. We could refactor this internally with some TempGrid or something, but I wanted to focus this PR on the API.

Note that (as mentioned in #26) this PR removes the ability to use fit_into_columns.

cc @PThorpe92

The corresponding change in uutils ls would look like this: https://github.com/uutils/coreutils/pull/5485/files

Removes ability to fit into columns. All cells must be given up front. The `Display` type is removed.

In uutils, we always fall back on printing everything in a single column if the width is too small, so we simply use that as the default.

PThorpe92 · 2023-11-01T13:12:17Z

Those codecov warnings make it really frustrating to look at diffs lol

Thanks for working with us on this. I'm testing this out now 👍

tertsdiepraam · 2023-11-01T13:21:36Z

You can turn the annotations off here!

Thanks for testing it out!

PThorpe92

Looks great!

tertsdiepraam · 2023-11-01T13:55:51Z

Thanks for testing!

I'll quickly mention that I want to do another iteration at some point that's gonna make this more zero-copy as well, but I'm still thinking about how to do that cleanly.

tertsdiepraam · 2023-11-01T15:37:39Z

Oops should have requested review instead of assigning :)

src/lib.rs

cakebaker

Good work :)

tertsdiepraam · 2023-11-02T11:48:54Z

I'm exploring some more improvements right now. Specifically, if we implement a proper width calculation, we don't need Cell anymore. There's a couple implementations out there:

Both are already part of the coreutils dependency tree. I'm tempted to use textwraps implementation, because it seems to be quite fast, because it does not create a new string and ignores the meaning of the ANSI codes.

README.md

examples/basic.rs

sylvestre · 2023-11-02T11:54:10Z

src/lib.rs

@@ -474,7 +344,8 @@ impl fmt::Display for Display<'_> {

 // Adapted from the unstable API:
 // https://doc.rust-lang.org/std/primitive.usize.html#method.div_ceil
-/// Division with upward rouding
+// Can be removed on MSRV 1.70.
+/// Division with upward rounding


Suggested change

/// Division with upward rounding

// Division with upward rounding

for consistency

The idea there was that the first few comments are internal, while the last one is the docstring. But it's not a public function, so I can make it consistent if you want.

sylvestre

some nits, feel free to ignore them

tertsdiepraam · 2023-11-02T12:03:53Z

Ok, @PThorpe92 I need you again :) I've made the pretty drastic change to remove Cell and I'm using the textwrap::core::display_width function to do the width calculation. This should hopefully remove the need to do any custom width calculations. It'd be great if you could confirm that this works for eza as well. Basically, you can now just give the Grid either &str or String and it should just work. It passes our test suite for ls at least, so I'm hopeful that it will work.

tertsdiepraam · 2023-11-02T12:10:06Z

(I have to update the docs to reflect this still)

tertsdiepraam · 2023-11-03T11:18:17Z

I've made even more changes and I think this would be enough for a new release. It passes both the uutils¹ and eza² test suites (huge thanks to @PThorpe92 for testing it!). But of course, these new changes still need to be reviewed.

README.md

examples/basic.rs

examples/big.rs

src/lib.rs

cakebaker · 2023-11-03T14:37:03Z

src/lib.rs

                let last_in_row = x == self.dimensions.widths.len() - 1;

                let col_width = self.dimensions.widths[x];
-                let padding_size = col_width - cell.width;
+                let padding_size = col_width - width;


This line can lead to a panic with "attempt to subtract with overflow". For example, if you set width: 15 in the basic example.

Should be fixed now! I set the width for the fallback to a single column incorrectly. Nice find! Btw, how did find this?

Right? damn I was gonna say... good catch lol

@tertsdiepraam I played a bit with the basic example and wanted to see what happens if the width is low/zero ;-)

tertsdiepraam · 2023-11-03T22:13:11Z

I still need to fix some of those suggestions. I'll get to that soon. I agree with all of them :)

examples/basic.rs

cakebaker

With the exception of the tiny detail in examples/basic.rs it looks good :)

allaboutevemirolive · 2023-12-01T14:34:05Z

Hye, @tertsdiepraam ! With this pull request, does it mean issue #24 should be closed, and others can work with issue #5396?

tertsdiepraam · 2023-12-01T16:39:06Z

@allaboutevemirolive If I recall correctly, that PR wasn't quite correct yet right? The functionality that we want with the tabs has not been implemented here yet, so if you still want to work on it, you can. Although, with all the refactoring that happened it might be easier to start over. Sorry for all those changes :)

tertsdiepraam added 2 commits October 31, 2023 16:45

simplify API

7d37528

Removes ability to fit into columns. All cells must be given up front. The `Display` type is removed.

remove Width type alias

bf3313a

tertsdiepraam force-pushed the api-changes branch from 7fee259 to bf3313a Compare October 31, 2023 15:46

This comment was marked as outdated.

Sign in to view

make Grid::new always succeed

b9e4252

In uutils, we always fall back on printing everything in a single column if the width is too small, so we simply use that as the default.

tertsdiepraam force-pushed the api-changes branch from 0600f4e to b9e4252 Compare October 31, 2023 19:41

tertsdiepraam assigned cakebaker and sylvestre Nov 1, 2023

PThorpe92 approved these changes Nov 1, 2023

View reviewed changes

tertsdiepraam unassigned cakebaker and sylvestre Nov 1, 2023

tertsdiepraam requested review from cakebaker and sylvestre November 1, 2023 15:37

cakebaker reviewed Nov 2, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

cakebaker reviewed Nov 2, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

cakebaker reviewed Nov 2, 2023

View reviewed changes

tertsdiepraam added 2 commits November 2, 2023 11:54

improve and update docstrings

8f1a32a

rename widest_cell_length to widest_cell_width

bc2a5ba

sylvestre reviewed Nov 2, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

sylvestre reviewed Nov 2, 2023

View reviewed changes

examples/basic.rs Outdated Show resolved Hide resolved

sylvestre reviewed Nov 2, 2023

View reviewed changes

sylvestre approved these changes Nov 2, 2023

View reviewed changes

tertsdiepraam force-pushed the api-changes branch from b5811de to 87ccb2f Compare November 2, 2023 12:02

fix width calculation and remove Cell

fe0a129

tertsdiepraam force-pushed the api-changes branch from 87ccb2f to fe0a129 Compare November 2, 2023 12:38

update and format README.md

0979c97

tertsdiepraam mentioned this pull request Nov 2, 2023

ls: upgrade uutils-term-grid to 0.5 uutils/coreutils#5485

Merged

tertsdiepraam requested review from cakebaker and sylvestre November 3, 2023 11:16