Merged
Conversation
Profpatsch
suggested changes
Sep 14, 2020
Member
Profpatsch
left a comment
There was a problem hiding this comment.
I love this, I’ve been wanting to have multiline toPretty in forever.
Two additional things:
- I’d leave the
if/then/elseform instead of going vialtypeOf, since it’s more straightforward and easier to adjust. - 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 …
7657cf4 to
3d014c5
Compare
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
9335702 to
15c5ba9
Compare
Member
Author
|
Did some changes:
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 |
Profpatsch
approved these changes
Sep 17, 2020
Member
Profpatsch
left a comment
There was a problem hiding this comment.
Looks very good to me 👍
Haven’t reviewed the logic in-depth, but we have tests so it’s fine. LGTM
10 tasks
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation for this change
For big Nix expressions,
toPrettycan be very hard to read, as it doesn't output any newlines. This PR implements themultilineoption fortoPretty, enabled by default, which makes it output newlines with proper indentation. Example output:In addition, I made some other improvements:
escapeNixIdentifier, introduced in f9eb3d1Ping @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