Skip to content

nix-cli: Add --json --pretty / --no-pretty#12652

Merged
roberth merged 2 commits into
NixOS:masterfrom
roberth:cli-json-pretty
Mar 18, 2025
Merged

nix-cli: Add --json --pretty / --no-pretty#12652
roberth merged 2 commits into
NixOS:masterfrom
roberth:cli-json-pretty

Conversation

@roberth

@roberth roberth commented Mar 14, 2025

Copy link
Copy Markdown
Member

Scripts are mostly unaffected because there, stdout will be a file or a pipe, not a terminal, so they get the old behavior.
(Of course scripts can be constructed that create terminals etc, as I did in the test, but taking that into account would be disproportionate.)

This refactors nix develop internals a bit to use the json type more. The assertion now operates in the in-memory json instead of re-parsing it. While this is technically a weaker guarantee, we should be able to rely on the library to get this right. It's its most essential purpose.

Motivation

Fix a repeating papercut.

Context


Add 👍 to pull requests you find important.

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

Default: istty(stdout)

This refactors `nix develop` internals a bit to use the `json` type
more. The assertion now operates in the in-memory json instead of
re-parsing it. While this is technically a weaker guarantee, we
should be able to rely on the library to get this right. It's its
most essential purpose.
@roberth roberth requested a review from edolstra as a code owner March 14, 2025 12:47
@github-actions github-actions Bot added new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority labels Mar 14, 2025

@crertel crertel left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM, couple of teeny spots to clean if desired.

Comment thread src/libmain/common-args.hh Outdated

This option is only effective when `--json` is also specified.
)",
//.category = commonArgsCategory,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
//.category = commonArgsCategory,

Remove deadcode.

Comment thread src/libmain/common-args.hh Outdated

See `--pretty`.
)",
//.category = commonArgsCategory,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
//.category = commonArgsCategory,

Remove deadcode.

Some four years old; time to go

@crertel crertel left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM!

Non-blocking question: I noticed that the logger invocations seem to have been centralized into how printJSON works now (if I read it correctly). Are we ever going to want to do anything file-specific or class-specific with the loggers, or is it okay that it seems like they all kinda share that implicitly now (or am I misunderstanding how logging works in nix)?

@roberth

roberth commented Mar 18, 2025

Copy link
Copy Markdown
Member Author

file-specific or class-specific with the loggers

Like log4j style logging? Nix'slogging is a bit simpler and more user focused than developer focused.
It might make sense to have more localized logger objects that can be configured to follow specific logging settings along the lines of "evaluation = notice; store = warn; interrupts = verbose;", but generally not something that exposes implementation file names, classes, etc to users.

@roberth roberth merged commit 95b0971 into NixOS:master Mar 18, 2025
@roberth

roberth commented Mar 18, 2025

Copy link
Copy Markdown
Member Author

Thank you @crertel for the review!

@crertel

crertel commented Mar 18, 2025

Copy link
Copy Markdown

Like log4j style logging? [...]

Ah, okay, thank you for the information! I wasn't sure, hence my question. Now I know. :)

@nixos-discourse

Copy link
Copy Markdown

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-2-29-0-released/64609/1

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

Labels

new-cli Relating to the "nix" command 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.

Indent JSON when stdout is terminal

3 participants