foundry icon indicating copy to clipboard operation
foundry copied to clipboard

feat: add `foundry_common::shell` to unify log behavior

Open zerosnacks opened this issue 1 year ago • 5 comments

Motivation

To unify the shell implementation for improved consistency.

Solution

  • Implements a common global shell for all output, correctly rendering to stdout on logs (sh_println!) and stderr on warnings and errors (sh_eprintln!, sh_warn!, sh_err!).
  • Adds a global --quiet / -q flag, easily checked against with shell::is_quiet() for conditional logic
  • Aims to keep the diff as low as possible whilst addressing the removal of existing methods like p_println, cli_warn, shell::println etc..

Changes from initial common shell implementation:

  • Macros now implement an unwrap directly on them, avoiding the need to pass / introduce ? anywhere.
  • sh_println! / sh_eprintln! / sh_warn! now all listen to the --quiet flag
  • Regular output logs are rendered with sh_println! as opposed to sh_eprintln!
  • Warnings are rendered to stderr output

To check / fix from https://github.com/foundry-rs/foundry/pull/6569:

  • [ ] Evaluate https://github.com/foundry-rs/foundry/issues/3981.
  • [ ] Evaluate https://github.com/foundry-rs/foundry/issues/2433.
  • [ ] Evaluate https://github.com/foundry-rs/foundry/issues/1976.
  • [ ] Evaluate https://github.com/foundry-rs/foundry/issues/7055.
  • [ ] Evaluate https://github.com/foundry-rs/foundry/issues/8568

Follow ups:

  • [ ] Introduce a global --json flag, similar to the global --quiet flag.
  • [ ] With shell::Format::Json enabled set the verbosity to quiet, disabling all output except for errors.
  • [ ] Introduce a sh_json! macro that renders JSON to the console if shell::Format::Json is active, either pretty or not. This could be automatically a no-op in anything except when the --json flag is passed.
  • [ ] Make sure all commands and modes are --json compatible, related: https://github.com/foundry-rs/foundry/issues/8794. With the current proposed change Anvil for example now has a --json flag but is not yet compatible. What --json means in context dependent to the command / tool. Goal is that it becomes much easier to chain Foundry into other tools.
  • [ ] Re-add additional macros like note and status and implement
  • [ ] Enable Clippy lint, search and replace.

zerosnacks avatar Oct 14 '24 13:10 zerosnacks

if possible, we can do this in smaller chunks because this pr is a conflict magnet

mattsse avatar Oct 14 '24 13:10 mattsse

if possible, we can do this in smaller chunks because this pr is a conflict magnet

Noted, I've tried to keep the changes to a minimum but due to the nature of the PR there are a lot of small changes across many files. I've not replaced any println!'s that are not strictly necessary (in regards to their expected behaviour w/ the --quiet flag). ~~One thing that is possibly not strictly necessary but I feel makes a lot of sense to add is the global --json flag as there is a lot of conditional logic dependent on it.~~ Removed the global --json flag for now.

zerosnacks avatar Oct 15 '24 16:10 zerosnacks

Moving back to draft, splitting out proposed global --json flag as it contained a breaking change and needs more consideration

zerosnacks avatar Oct 16 '24 10:10 zerosnacks

Applied fixes from review cc @DaniPopes, would you mind checking one more time?

To avoid visual regression, applied a change to style warning message content in yellow and error message content in red.

zerosnacks avatar Oct 17 '24 10:10 zerosnacks

Applied a minor fix for conditional rendering of failed tests when --junit flag is passed to avoid modifying the global shell behavior

zerosnacks avatar Oct 17 '24 16:10 zerosnacks

To avoid visual regression, applied a change to style warning message content in yellow and error message content in red.

This is not right, only the "warn"/"error" should be styled

Reverted the style change as result of internal poll, now renders as follows again:

Screenshot from 2024-10-21 12-09-44

zerosnacks avatar Oct 21 '24 11:10 zerosnacks