fix: implement EZA_GRID_ROWS grid details view minimum rows threshold

[LeoniePhiline]

Grid details view had been prevented only by console width being unavailable.

This changeset implements EZA_GRID_ROWS as secondary grid details inhibitor, preventing grid details view if the minimum rows threshold is not reached by the grid which would be rendered.

Fixes complaint at #66 (comment)

Side note: EZA_GRID_ROWS only applies to --long --grid, but not to non-details --grid view. Looking at the code and documentation, this appears to be intentional.

[LeoniePhiline]

The failure of Flake Checker appears unrelated.

Edit: That's DeterminateSystems/flake-checker-action#32.

[PThorpe92]

This looks good 👍 thanks for the PR!

I do have a question as far as the expected output, (just fyi I could be way off here as it's late and I just skimmed over the issue.)

I would assume that rows would be talking about the individual rows created with N columns after the grid is rendered. This checks if files.len() < minimum rows. But because the grid could calculate N columns, when I set EZA_GRID_ROWS to 5, it could still render a grid with 1 or 2 'rows'. Is this expected?
e.g.

My understanding is it would render N columns where the rows >= EZA_GRID_ROWS. Like I said I could be off. I'll take another look tomorrow after work 👍

[LeoniePhiline]

You could indeed be very right in that the configured rows refers to the rows in the grid view rather than the rows in the list view.

This means I would need to divide the number of files by the number of grid columns and round up.

The config setting should then be interpreted as "Render a details grid, but only if the grid is going to be long enough".

Maybe we could also improve the documentation of the environment variable to make this ambiguity clearer.

[PThorpe92]

You could indeed be very right in that the configured rows refers to the rows in the grid view rather than the rows in the list view.

Ok good I thought I was missing something 😅. but you are right it's definitely because of the ambiguity. The only reason I think may be correct, is the var seems to be discussed in the context of grid, in which case a row would be a grid row.

Some clearer docs would be helpful for sure 👍 (assuming that is the case anyway, someone else let me know if this is off base)

[LeoniePhiline]

I've adjusted the implementation. It now entirely lives within src/output/grid_details.rs, since the grid dimensions are determined only late in the pre-rendering process.

I re-read the documentation of EZA_GRID_ROWS and found it sufficiently clear. I just hadn't read it properly previously.

The algorithm works correctly now:

The second invocation renders a details list view, since the grid would not at least span 11 rows.

[LeoniePhiline]

@PThorpe92 PTAL 😄

[PThorpe92]

heyy sorry man, super busy week.

Awesome! this looks good to me 👍

Thanks for the PR!

p.s. so you think the existing docs are sufficient, and we had just skimmed it over?

all set w/ me, can merge when @cafkafk is ready

[LeoniePhiline]

heyy sorry man, super busy week.

No worries, I just wasn't sure if you had been notified about the PR update. Hope it's okay that I pinged you!

Awesome! this looks good to me 👍

Thanks for the PR!

Thanks and very welcome!

p.s. so you think the existing docs are sufficient, and we had just skimmed it over?

It appeared to me like I merely skimmed it over and thus misunderstood. The docs seem sufficient to me as they are.

If you disagree, though, then please let me know. I can also contribute an update to the docs while we're at it.

[sabrehagen]

LGTM ✅

[sabrehagen]

LGTM

[LeoniePhiline]

Do you require a rebase or is this ready to merge as is?

[PThorpe92]

Yeah could you rebase from main? I just pinged cafk on matrix about it, she will take a look + merge but rebasing would be helpful.

[cafkafk]

Okay, this does look like it's working as expected. It's a breaking change, so I've changed to commit summary to reflect that.

Thanks for fixing this!