Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 47 additions & 9 deletions src/nix/flake.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "eval-cache.hh"
#include "markdown.hh"
#include "users.hh"
#include "terminal.hh"

#include <nlohmann/json.hpp>
#include <iomanip>
Expand Down Expand Up @@ -1243,25 +1244,62 @@ struct CmdFlakeShow : FlakeCommand, MixJSON
auto showDerivation = [&]()
{
auto name = visitor.getAttr(state->sName)->getString();
std::optional<std::string> description;
if (auto aMeta = visitor.maybeGetAttr(state->sMeta)) {
if (auto aDescription = aMeta->maybeGetAttr(state->sDescription))
description = aDescription->getString();
}

if (json) {
std::optional<std::string> description;
if (auto aMeta = visitor.maybeGetAttr(state->sMeta)) {
if (auto aDescription = aMeta->maybeGetAttr(state->sDescription))
description = aDescription->getString();
}
j.emplace("type", "derivation");
j.emplace("name", name);
if (description)
j.emplace("description", *description);
} else {
logger->cout("%s: %s '%s'",
headerPrefix,
auto type =
attrPath.size() == 2 && attrPathS[0] == "devShell" ? "development environment" :
attrPath.size() >= 2 && attrPathS[0] == "devShells" ? "development environment" :
attrPath.size() == 3 && attrPathS[0] == "checks" ? "derivation" :
attrPath.size() >= 1 && attrPathS[0] == "hydraJobs" ? "derivation" :
"package",
name);
"package";
if (description && !description->empty()) {
// Maximum length to print
size_t maxLength = getWindowSize().second > 0 ? getWindowSize().second : 80;

// Trim the description and only use the first line
auto trimmed = trim(*description);
auto newLinePos = trimmed.find('\n');
auto length = newLinePos != std::string::npos ? newLinePos : trimmed.length();

// Sanitize the description and calculate the two parts of the line
// In order to get the length of the printable characters we need to
// filter out escape sequences.
auto beginningOfLine = fmt("%s: %s '%s'", headerPrefix, type, name);
auto beginningOfLineLength = filterANSIEscapes(beginningOfLine, true).length();
auto restOfLine = fmt(" - '%s'", filterANSIEscapes(trimmed, false, length));

// If we are already over the maximum length then do not trim
// and don't print the description (preserves existing behavior)
if (beginningOfLineLength >= maxLength) {
logger->cout("%s", beginningOfLine);
}
else {
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, "...");
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. check length without codes
  2. start with string with codes
  3. subtract the difference from the max
  4. subtract a additional known amount as correction
  5. pad with a "reset" ANSI sequence
  6. ????
  7. Happiness

@kjeremy kjeremy Aug 9, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if filterANSIEscapes filtered out color codes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@roberth roberth Aug 14, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be great

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roberth Are we good to go as-is?

logger->cout("%s", line);
}
}
else {
logger->cout("%s: %s '%s'", headerPrefix, type, name);
}
}
};

Expand Down
25 changes: 25 additions & 0 deletions tests/functional/flakes/show.sh
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,28 @@ assert show_output.legacyPackages.${builtins.currentSystem}.AAAAAASomeThingsFail
assert show_output.legacyPackages.${builtins.currentSystem}.simple.name == "simple";
true
'

cat >flake.nix<<EOF
{
outputs = inputs: {
packages.$system = {
aNoDescription = import ./simple.nix;
bOneLineDescription = import ./simple.nix // { meta.description = "one line"; };
cMultiLineDescription = import ./simple.nix // { meta.description = ''
line one
line two
''; };
dLongDescription = import ./simple.nix // { meta.description = ''
01234567890123456789012345678901234567890123456789012345678901234567890123456789abcdefg
''; };
eEmptyDescription = import ./simple.nix // { meta.description = ""; };
};
};
}
EOF
nix flake show > ./show-output.txt
test "$(awk -F '[:] ' '/aNoDescription/{print $NF}' ./show-output.txt)" = "package 'simple'"
test "$(awk -F '[:] ' '/bOneLineDescription/{print $NF}' ./show-output.txt)" = "package 'simple' - 'one line'"
test "$(awk -F '[:] ' '/cMultiLineDescription/{print $NF}' ./show-output.txt)" = "package 'simple' - 'line one'"
test "$(awk -F '[:] ' '/dLongDescription/{print $NF}' ./show-output.txt)" = "package 'simple' - '012345678901234567890123456..."
test "$(awk -F '[:] ' '/eEmptyDescription/{print $NF}' ./show-output.txt)" = "package 'simple'"