Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve generators.toPretty #97133

Merged
merged 7 commits into from Sep 21, 2020
Merged

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Sep 4, 2020

Motivation for this change

For big Nix expressions, toPretty can be very hard to read, as it doesn't output any newlines. This PR implements the multiline option for toPretty, enabled by default, which makes it output newlines with proper indentation. Example output:

{
  foo = {
    bar = 10;
    baz = [
      "baz"
    ];
  };
}

In addition, I made some other improvements:

  • Don't quote attribute names if they don't need to be quoted by using escapeNixIdentifier, introduced in f9eb3d1
  • Switch away from the δ and λ symbols for derivations and functions respectively
  • Multiline strings are printed correctly now

Ping @Profpatsch @rycee

Best reviewed commit-by-commit. Specifically there's one commit in there that just refactors the function without changing its behavior.

Things done
  • Added a new test case for all functionality

Copy link
Member

@Profpatsch Profpatsch left a comment

Choose a reason for hiding this comment

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

I love this, I’ve been wanting to have multiline toPretty in forever.

Two additional things:

  1. I’d leave the if/then/else form instead of going vial typeOf, since it’s more straightforward and easier to adjust.
  2. I’d render lists and attrsets that only have one element on one line to be slightly more compact. From 2 elements, split onto newlines. Though that might be easier said than done …

lib/generators.nix Outdated Show resolved Hide resolved
lib/tests/misc.nix Outdated Show resolved Hide resolved
lib/generators.nix Outdated Show resolved Hide resolved
As a preparation to the following commit
- These symbols can be confusing for those not familiar with them
- There's no harm in making these more obvious
- Terminals may not print them correctly either

Also changes the function argument printing slightly to be more obvious
Not attribute sets. So move the function case forward
@infinisil
Copy link
Member Author

Did some changes:

  • Multiline strings are printed correctly now
  • Switch away from the δ and λ symbols for derivations and functions respectively
  • Went back to the big if else. I don't think it's nicer, but it works out well for another change:
  • To print __functor's as functions
  • Removed the prefix argument again. This can be achieved with builtins.replaceStrings [ "\n" ] [ "\nprefix" ] as well

I decided to not render single-element lists/attrs without newlines, because I fear this makes it more complicated to understand the output, as the indentation can't be used to see the nesting level anymore.

Here's a sample output for this Nix expression:

{
  value = {
  fun = lib.id;
  funargs = lib.types.submoduleWith;
  multiline = {
    multi = ''
      hello
      there!
    '';
    multi' = ''
      hello
      there!'';
    single = "hello there!";
  };
  pkg = pkgs.hello;
  single = { foo = [ { bar = [ null ]; } ]; };
}

Which renders as

{
  fun = <function>;
  funargs = <function, args: {modules, shorthandOnlyDefinesConfig?, specialArgs?}>;
  multiline = {
    multi = ''
      hello
      there!
    '';
    multi' = ''
      hello
      there!'';
    single = "hello there!";
  };
  pkg = <derivation /nix/store/2nnf3klixf4j2ih8x0zx4albfzlk74l9-hello-2.10.drv>;
  single = {
    foo = [
      {
        bar = [
          null
        ];
      }
    ];
  };
}

Copy link
Member

@Profpatsch Profpatsch left a comment

Choose a reason for hiding this comment

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

Looks very good to me 👍

Haven’t reviewed the logic in-depth, but we have tests so it’s fine. LGTM

@infinisil infinisil merged commit 366a677 into NixOS:master Sep 21, 2020
@infinisil infinisil deleted the improved-toPretty branch September 21, 2020 15:11
@edolstra
Copy link
Member

I suppose eventually we'd want this to be a builtin function in Nix, since it's needed in the UI anyway, and it's easier on the C++ side to handle circular values etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants