nix flake show: add the description if it exists #10980

kjeremy · 2024-06-27T19:42:04Z

Motivation

Displays the description of the package if it exists. This information is already present in the json output.

I have a flake with many packages and dev shells and this helps discoverability.

Context

#10977

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

kjeremy · 2024-07-01T13:32:03Z

This has been really helpful for my team. What do I need to do to get it merged?

edolstra · 2024-07-01T14:05:25Z

The problem with showing descriptions in nix flake show is that they can be pretty long, which can break the tree output rendering (especially if they contain newlines).

kjeremy · 2024-07-01T14:25:51Z

@edolstra I can think of a few approaches:

Strip newlines and maybe limit the length
Fix the tree views to work with multiline output

I can tackle either one if I get some direction.

kjeremy · 2024-07-08T17:18:41Z

I've pushed a change to stop at the first new line

kjeremy · 2024-07-08T17:41:49Z

I'm not quite sure how to test the output. The tests in tests/functional/flakes/show.sh all depend on the json output but this needs to check stdout.

cole-h · 2024-07-08T17:56:03Z

As a bystander, I think the following implementation might make the most sense:

Strip all whitespace from the beginning of the description until you get to a non-whitespace character
Read up to the next newline (if any)
If the resulting string is longer than, say, 100 characters, trim it to that and add an ellipsis at the end

I'm not quite sure how to test the output. The tests in tests/functional/flakes/show.sh all depend on the json output but this needs to check stdout.

You could try using <the command> | grepQuiet "what you're looking for", I think.

kjeremy · 2024-07-08T20:32:18Z

I chose 80 and added tests.

kjeremy · 2024-07-10T13:47:12Z

@edolstra is this a reasonable solution to the tree rendering?

src/nix/flake.cc

kjeremy · 2024-07-23T12:50:01Z

@Ericson2314 is there any thing I can do to move this along?

(cherry picked from commit 8cd1d02f90eb9915e640c5d370d919fad9833c65) nix flake show: Only print up to the first new line if it exists. (cherry picked from commit 5281a44927bdb51bfe6e5de12262d815c98f6fe7) add tests (cherry picked from commit 74ae0fbdc70a5079a527fe143c4832d1357011f7) Handle long strings, embedded new lines and empty descriptions (cherry picked from commit 2ca7b3afdbbd983173a17fa0a822cf7623601367) Account for total length of 80 (cherry picked from commit 1cc808c18cbaaf26aaae42bb1d7f7223f25dd364) docs: add nix flake show description release note fix: remove white space nix flake show: trim length based on terminal size test: account for terminal size docs(flake-description): before and after commands; add myself to credits Upstream-PR: NixOS/nix#10980 Change-Id: Ie1c667dc816b3dd81e65a1f5395e57ea48ee0362

kjeremy · 2024-08-05T14:29:11Z

It looks like lix just merged this into their code base.

edolstra · 2024-08-05T14:56:37Z

src/nix/flake.cc

-                            name);
+                            "package";
+                        if (description && !description->empty()) {
+                            // Trim the string and only display the first line of the description.


This can be done using filterANSIEscapes():

/** * Truncate a string to 'width' printable characters. If 'filterAll' * is true, all ANSI escape sequences are filtered out. Otherwise, * some escape sequences (such as colour setting) are copied but not * included in the character count. Also, tabs are expanded to * spaces. */ std::string filterANSIEscapes(std::string_view s, bool filterAll = false, unsigned int width = std::numeric_limits<unsigned int>::max());

I.e. something like filterANSIEscapes(s, false, getWindowSize().second()).

@edolstra At this point getWindowSize() returns 0,0. I tried calling updateWindowSize() manually but I keep getting 0,0 in the test.

Could be running headless in tests. Does it work when calling it interactive?

Ah yes, I see values if I run nix flake show on the console. Do I need to provide a maximum length for headless mode? If so then what is the recommended way to detect it (isTTY?)?

Pretty nice would be to output relative to current terminal? (Or 80-ish if headless)

Pretty nice would be to output relative to current terminal? (Or 80-ish if headless)

I think that's what the getWindowSize approach is supposed to do unless I'm misunderstanding something.

Exactly, I was thinking that it would call getWindowSize, if 0, use 80-ish, otherwise use what was returned. Be careful about some pathological tiny windows!

kjeremy · 2024-08-05T19:41:49Z

I think the macos test failure is spurious

tomberek · 2024-08-07T02:44:32Z

I tried this out and I still get overflow. Am I reading the logic wrong? Shouldn't the window size limitation be on the entire string rather than only the desc?

logger->cout("%s: %s '%s' - '%s'", headerPrefix, type, name, desc);
             ^-----------------^

kjeremy · 2024-08-07T12:00:06Z

I guess it could be. I was thinking it should just be on the description but maybe it should be on the whole string but we should only cut off the description if too long to match existing behavior

kjeremy · 2024-08-08T18:52:38Z

src/nix/flake.cc

+                                auto line = beginningOfLine + restOfLine;
+                                // FIXME: Specifying `true` here gives the correct length
+                                // BUT removes colors/bold so something is not quite right here.
+                                line = filterANSIEscapes(line, true, maxLength);
+
+                                // NOTE: This test might be incorrect since I get things like:
+                                // 168 or 161 > maxLength.
+                                if (line.length() > maxLength) {
+                                    line = line.replace(line.length() - 3, 3, "...");
+                                }


See the FIXME: here. I think this is pretty close but it's not 100% correct yet. filterANSIEscapes(..., true) seems to give a reasonable character count but I'm losing color information. filterANSIEscapes(..., false) gives funky length values so I think I'm missing something.

@edolstra @tomberek it's close! but I'm missing something with the escapes:

there might be a known number of escapes that you need to correct for, but then also use the original line (plus some terminating ANSI codes). Perhaps do:

check length without codes

start with string with codes

subtract the difference from the max

subtract a additional known amount as correction

pad with a "reset" ANSI sequence

????

Happiness

This is all fairly terrible. The filtering function definitely doesn't strip ascii codes out and this all depends on the nested level.

Is there a less perfect solution we could implement for now?

It would be nice if filterANSIEscapes filtered out color codes.

The new version uses a modified version of filterANSIEscapes with the tree characters taken into account. It's kind of ugly but seems to work!

This is all fairly terrible.

Absolutely. Terminal color logic, layout logic and flake attribute logic should all be separated out into distinct functions, classes, etc.
However, since we know the solution for the tech debt and we're not dealing with crazy compatibility requirements in this part of the code, I'd be happy to merge this and do the refactoring after.

That would be great

@roberth Are we good to go as-is?

tomberek · 2024-08-18T20:31:39Z

Tested and works as expected. Thanks!

…ption" This reverts commit 1c5ad15, reversing changes made to 67de193.

…ption" This reverts commit 1c5ad15, reversing changes made to 67de193. This reverts commit ce4e4a1, reversing changes made to 43e82c9.

Revert "Merge pull request #10980 from kjeremy/flake-show-description"

kjeremy requested a review from edolstra as a code owner June 27, 2024 19:42

github-actions bot added the new-cli Relating to the "nix" command label Jun 27, 2024

Ericson2314 added the flakes label Jul 5, 2024

kjeremy force-pushed the flake-show-description branch from ff1c83b to 496e6d5 Compare July 8, 2024 15:58

github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Jul 8, 2024

kjeremy force-pushed the flake-show-description branch from fdafe03 to 868f759 Compare July 8, 2024 19:28

cole-h mentioned this pull request Jul 17, 2024

feat: Add description to nix flake show #11129

Closed

fzakaria reviewed Jul 17, 2024

View reviewed changes

src/nix/flake.cc Outdated Show resolved Hide resolved

kjeremy force-pushed the flake-show-description branch 2 times, most recently from 74da3f4 to 1cc808c Compare July 18, 2024 18:44

kjeremy requested a review from fzakaria August 5, 2024 14:25

edolstra reviewed Aug 5, 2024

View reviewed changes

kjeremy added 6 commits August 5, 2024 11:02

nix flake show: add the description if it exists

547e808

nix flake show: Only print up to the first new line if it exists.

07d0527

add tests

59b6aaf

Handle long strings, embedded new lines and empty descriptions

f22cf1f

Account for total length of 80

930818b

copy string using filterANSIEscapes and enforce the max length

1c5f1de

kjeremy force-pushed the flake-show-description branch from 1cc808c to 1c5f1de Compare August 5, 2024 18:15

Use window size

9bf6684

Use the window size for the entire length

abbaba9

kjeremy commented Aug 8, 2024

View reviewed changes

kjeremy marked this pull request as draft August 8, 2024 18:52

Take ANSI and tree characters into account

d49e14b

kjeremy marked this pull request as ready for review August 12, 2024 18:52

kjeremy requested review from tomberek and edolstra August 12, 2024 20:00

kjeremy mentioned this pull request Aug 14, 2024

nix flake show: Support meta attribute for apps #11297

Merged

kjeremy requested a review from roberth August 17, 2024 13:33

tomberek merged commit 1c5ad15 into NixOS:master Aug 18, 2024
12 checks passed

kjeremy deleted the flake-show-description branch August 18, 2024 22:46

kjeremy mentioned this pull request Aug 19, 2024

nix flake show should show package descriptions if available #10977

Closed

edolstra mentioned this pull request Sep 18, 2024

aarch64-linux build is broken #11536

Open

edolstra added a commit to DeterminateSystems/nix-src that referenced this pull request Nov 7, 2024

Revert "Merge pull request NixOS#10980 from kjeremy/flake-show-descri…

9e51b4a

…ption" This reverts commit 1c5ad15, reversing changes made to 67de193.

edolstra added a commit that referenced this pull request Nov 7, 2024

Merge pull request #11829 from DeterminateSystems/revert-10980

fbceb86

Revert "Merge pull request #10980 from kjeremy/flake-show-description"

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nix flake show: add the description if it exists #10980

nix flake show: add the description if it exists #10980

kjeremy commented Jun 27, 2024

kjeremy commented Jul 1, 2024

edolstra commented Jul 1, 2024

kjeremy commented Jul 1, 2024

kjeremy commented Jul 8, 2024

kjeremy commented Jul 8, 2024 •

edited

Loading

cole-h commented Jul 8, 2024

kjeremy commented Jul 8, 2024

kjeremy commented Jul 10, 2024

kjeremy commented Jul 23, 2024

kjeremy commented Aug 5, 2024

edolstra Aug 5, 2024

kjeremy Aug 5, 2024 •

edited

Loading

tomberek Aug 5, 2024

kjeremy Aug 5, 2024 •

edited

Loading

tomberek Aug 5, 2024

kjeremy Aug 5, 2024

tomberek Aug 5, 2024

kjeremy commented Aug 5, 2024

tomberek commented Aug 7, 2024 •

edited

Loading

kjeremy commented Aug 7, 2024

kjeremy Aug 8, 2024

kjeremy Aug 8, 2024

tomberek Aug 8, 2024

kjeremy Aug 9, 2024 •

edited

Loading

kjeremy Aug 9, 2024

kjeremy Aug 9, 2024

kjeremy Aug 12, 2024

roberth Aug 14, 2024 •

edited

Loading

kjeremy Aug 14, 2024

tomberek Aug 16, 2024

tomberek commented Aug 18, 2024

nix flake show: add the description if it exists #10980

nix flake show: add the description if it exists #10980

Conversation

kjeremy commented Jun 27, 2024

Motivation

Context

Priorities and Process

kjeremy commented Jul 1, 2024

edolstra commented Jul 1, 2024

kjeremy commented Jul 1, 2024

kjeremy commented Jul 8, 2024

kjeremy commented Jul 8, 2024 • edited Loading

cole-h commented Jul 8, 2024

kjeremy commented Jul 8, 2024

kjeremy commented Jul 10, 2024

kjeremy commented Jul 23, 2024

kjeremy commented Aug 5, 2024

Choose a reason for hiding this comment

kjeremy Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjeremy Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjeremy commented Aug 5, 2024

tomberek commented Aug 7, 2024 • edited Loading

kjeremy commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjeremy Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberth Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomberek commented Aug 18, 2024

kjeremy commented Jul 8, 2024 •

edited

Loading

kjeremy Aug 5, 2024 •

edited

Loading

kjeremy Aug 5, 2024 •

edited

Loading

tomberek commented Aug 7, 2024 •

edited

Loading

kjeremy Aug 9, 2024 •

edited

Loading

roberth Aug 14, 2024 •

edited

Loading