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

quiet by default? #295

Closed
robx opened this issue May 31, 2022 · 17 comments
Closed

quiet by default? #295

robx opened this issue May 31, 2022 · 17 comments
Labels
component | engine Core algorithm of the tool status | it is a good thing We agree it is good to implement this type | feature request New feature or request

Comments

@robx
Copy link

robx commented May 31, 2022

I just tried out the new nix fmt with alejandra after upgrading to nixos 22.05, by setting

formatter.${system} = pkgs.alejandra;

It's almost great, except the excessive colored console output makes me have second thoughts. What do you think of setting it up such that it runs with -q by default? Essentially, I'd like nix fmt to have no output at all if my files are in order.

@winterqt
Copy link

winterqt commented Jun 29, 2022

@kamadorueda Do you mind if I PR a change prototyping what I think this should look like, to further discussion? (I ask since you haven't replied to this.) Ah, the existing quiet mode already implements all the UI changes I had in mind.

@winterqt
Copy link

@robx You can always use makeWrapper to wrap the binary so nix fmt runs Alejandra in quiet mode.

@kamadorueda
Copy link
Owner

In short, something like:

{
      formatter.${system} = nixpkgs.writeShellScriptBin "alejandra" ''
        exec ${nixpkgs.alejandra}/bin/alejandra --quiet "$@"
      '';
}

We would need a survey in order to know what mode (tui vs cli vs quiet) is preferred and make it the default

I personally chose to use tui as default because the progress bar helps a lot when formatting large codebases, but I understand why the quiet philosophy is valuable to some people, too

@kamadorueda kamadorueda added type | feature request New feature or request status | needs discussing Needs discussion component | engine Core algorithm of the tool labels Jun 30, 2022
@jessestricker
Copy link

I use alejandra for all my Nix projects, it's really great!

Like @robx, this is my only issue with the application, so I think having a survey on the default behaviour would be nice.

Personally, I prefer the quiet mode:

  • it does not clear the screen (this is the biggest issue)
  • it makes alejandra more in-line with other formatters (and CLI tools in general)
  • a progress bar is not that useful in small projects anyways (how often do you run alejandra on the whole of nixpkgs?)

@kamadorueda
Copy link
Owner

I just created some poll over here: https://discourse.nixos.org/t/the-uncompromising-nix-code-formatter/17385/48?u=kamadorueda

And it seems this is actually a good idea, not to mention other people also support this point of view: https://en.wikipedia.org/wiki/Unix_philosophy
So I'll be drafting a change once I get some spare time, thanks for the feedback!

@kamadorueda kamadorueda added status | it is a good thing We agree it is good to implement this and removed status | needs discussing Needs discussion labels Aug 3, 2022
@kamadorueda
Copy link
Owner

Some updates to the CLI just landed master, it would be good if you help me review them before creating a release, with them, this issue could be closed

@robx @jessestricker @winterqt

@robx
Copy link
Author

robx commented Aug 6, 2022

I see this now, without a --quiet wrapper shell script:

$ nix fmt
Checking style in 6 files using 2 threads.


Congratulations! Your code complies the Alejandra style.

If Alejandra is useful to you, please sponsor its author.
Thank you! https://github.com/sponsors/kamadorueda

@jessestricker
Copy link

jessestricker commented Aug 6, 2022

👍🏾 I think the new default is an improvement.

I would have preferred it to be like this though:

  • default mode: no success output, just errors (like --quiet)
  • --verbose: list of formatted files, success output, also errors (like new default)
  • --verbose --verbose: the TUI mode (like old default)

If you agree, I'd be willing to do a PR for this.

@SignalWalker
Copy link

SignalWalker commented Aug 6, 2022

I've been using this to format neovim buffers with :%!alejandra -q; this used to work perfectly, but, in this version, the donation/star ads are sometimes appended to the end of output, which breaks things -- i.e. :%!alejandra -q is no longer idempotent, because it will sometimes append an ad to an otherwise unchanged input.

@winterqt
Copy link

winterqt commented Aug 6, 2022

I don't see why it would be only sometimes, but this looks to be the culprit, it's right outside of the quiet check:

if in_place {
println!();
print!("{}", random_ad());
}

@kamadorueda
Copy link
Owner

Thanks! let me fix it

@winterqt
Copy link

winterqt commented Aug 6, 2022

Also, @kamadorueda: is there a reason that the ads here are printed to stdout, when otherwise the status messages are printed to stderr? Seems like it could also break things.

if !args.quiet {
eprintln!();
eprintln!("Congratulations! Your code complies the Alejandra style.");
println!();
print!("{}", random_ad());
}

@kamadorueda
Copy link
Owner

No, I must have been distracted when I implemented that, fix is here: #340

@kamadorueda
Copy link
Owner

@SignalWalker I just fixed that:

[kamadorueda@machine:/data/alejandra]$ echo {} | cargo -q run -- -q
{}

[kamadorueda@machine:/data/alejandra]$ echo $?
0

[kamadorueda@machine:/data/alejandra]$ echo {}} | cargo -q run -- -q
{}}

Failed! 1 error found at:
- <anonymous file on stdin>: unexpected token at 2..3

[kamadorueda@machine:/data/alejandra]$ echo $?
1

Do you think printing the error when formatting stdin is useful? I just tried it in Vim and the error message is injected into the buffer, which then requires an 'undo' to revert the change

Would one of the following be helpful for Vim users?

  • Do not print the error messages when receiving input from stdin?
  • Make -qq also suppress error messages

@SignalWalker
Copy link

-qq would be useful, yeah. Thanks for the fix!

@kamadorueda
Copy link
Owner

kamadorueda commented Aug 15, 2022

I just released 3.0.0

While this issue was not exactly implemented as initially proposed, the new CLI output contains the elements the community cares about the most and the identified pain points were fixed

@pacak
Copy link

pacak commented Jul 14, 2023

FWIW configuring according to https://nixos.org/manual/nix/stable/command-ref/new-cli/nix3-fmt.html (included below) makes it not quiet.

# flake.nix
{
  outputs = { nixpkgs, self }: {
    formatter.x86_64-linux = nixpkgs.legacyPackages.x86_64-linux.alejandra;
  };
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component | engine Core algorithm of the tool status | it is a good thing We agree it is good to implement this type | feature request New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants