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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

libexpr: extend Value::print to allow limited depth #8566

Merged
merged 2 commits into from Jul 1, 2023

Conversation

inclyc
Copy link
Member

@inclyc inclyc commented Jun 22, 2023

Motivation

This is an extension for setting limited depth for nix::Value printing. Sometimes this list is fairly large because of recursive set inside nix, and printing in a desired depth make things much easier for users who just want to see limited depth (for example, when using nix repl, we may only want to see depth = 1 attribute fields, instead of digging into it (basically unreadable). Maybe we can extend nix repl after this change, for limited depth printing.

Context

I'm writting a nix language server which would like to provide consistent developer experience between editors and official nix implementation, and nix::Value::print seems a good way to show values (there is no other good public method to do so). And this method may take long time to finish.

Related PR: nix-community/nixd#149

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 馃憤 to pull requests you find important.

@inclyc inclyc requested a review from edolstra as a code owner June 22, 2023 04:38
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

I'd say a depth parameter is a last resource, but a valid solution that's fairly easy to implement and maintain. You might want to consider, although you probably already have:

  1. only explore one layer at a time, lazily, on-demand, interactive, etc
  2. a mix of "depth first" and "breadth first" until you reach an output size limit; some clever balancing act. Should work reasonably even for pkgs.
  3. depth limit. The minimum limit is still terrible for something like pkgs

Assuming 1 is not feasible for you and 2 is a "research project" and a lot more to test and review, let's go ahead with this.

This needs testing. Ideally I'd like to see a new unit test in src/libexpr/tests (I hate to say that this function is not unit tested yet), but perhaps it would also be nice to have a depth parameter for nix-instantiate, which uses this method.

src/libexpr/eval.cc Outdated Show resolved Hide resolved
@inclyc
Copy link
Member Author

inclyc commented Jun 22, 2023

Thanks for reviewing my PRs 鉂わ笍 .

a mix of "depth first" and "breadth first" until you reach an output size limit; some clever balancing act. Should work reasonably even for pkgs.

Before addressing review comments, would you like to accept patches that trying to stop printing to ostream if it reaches some size limit also? Because pkgs is fairly big even when depth = 1.

I'd say a depth parameter is a last resource, but a valid solution that's fairly easy to implement and maintain.

Yes, it gives us reasonable outputs because nix language itself is structural and nested, so given a "depth" argument somehow preserve enough information instead of digging in some irrelavent attribute paths.

This needs testing. Ideally I'd like to see a new unit test in src/libexpr/tests (I hate to say that this function is not unit tested yet), but perhaps it would also be nice to have a depth parameter for nix-instantiate, which uses this method.

Thanks, I would like to see how to add these tests then.

@roberth
Copy link
Member

roberth commented Jun 22, 2023

Thanks, I would like to see how to add these tests then.

You can refer to the existing tests as examples of how to set them up.

  • nix-instantiate is tested in tests/eval.sh
  • evaluator unit tests are in src/libexpr/tests

@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Jun 22, 2023
@inclyc inclyc requested a review from roberth June 23, 2023 14:09
@github-actions github-actions bot added the tests label Jul 1, 2023
@roberth roberth merged commit 7b39a38 into NixOS:master Jul 1, 2023
10 checks passed
@inclyc inclyc deleted the nixd/value-print-depth branch July 1, 2023 20:58
@fricklerhandwerk fricklerhandwerk added feature Feature request or proposal repl The Read Eval Print Loop, "nix repl" command and debugger labels Jan 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request or proposal repl The Read Eval Print Loop, "nix repl" command and debugger tests with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants