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

main: extract a Status object to represent the final status of nixpkgs-check-by-name #91

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

philiptaron
Copy link
Contributor

Motivation for changes

I didn't like the mixing of error reporting (through io::Writer) and business logic inside the process function. By extracting a Status object, the two are separated, just like NixpkgsProblem did on a lower level.

Description of changes

  • Extracted Status object which contains the actual status of nixpkgs-check-by-name checks.
  • Isolated coloring decisions to private Status::fmt method.
  • Isolated ExitCode transformation to From impl.
  • Introduced ColoredStatus which is the colored version of status; it's used exclusively in main.
  • All tests continue to pass without the NO_COLOR environment variable and test-only io::Writer.

It's mildly longer (~53 more lines) due to the needed boilerplate of being a module.

@philiptaron philiptaron requested a review from a team as a code owner July 27, 2024 05:33
@willbush
Copy link
Member

Damn man your killin it today!

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

This is a nice improvement overall!

src/status.rs Outdated Show resolved Hide resolved
src/status.rs Outdated
Comment on lines 33 to 35
let maybe_green = |s: &str| if use_color { s.green() } else { s.normal() };
let maybe_yellow = |s: &str| if use_color { s.yellow() } else { s.normal() };
let maybe_red = |s: &str| if use_color { s.red() } else { s.normal() };
Copy link
Member

Choose a reason for hiding this comment

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

This will cause it to colorise the output even if it's e.g. just a pipe to a file. So I think I prefer the previous code using NO_COLOR=1 to disable colors when testing, and https://github.com/NixOS/nixpkgs/blob/07c1a652be3a1dda9047f4d8545e17d12958a148/.github/workflows/check-by-name.yml#L113-L115 to force turn it on in CI.

Copy link
Contributor Author

@philiptaron philiptaron Jul 27, 2024

Choose a reason for hiding this comment

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

No, it's the same as before.

nixpkgs-check-by-name used to unilaterally colorize by calling the .red, .yellow, etc methods, and in the colored crate it would decide to actually not color based on this environment variable.

Now, it'll only call those methods if true is passed to use_color. ColoredStatus does this -- see the end of the file. main uses ColoredStatus, and so it retains the original behavior. Tests, which use the process method, format a Status, which passes false to use_color in their display, bypassing colorization in this module itself.

I like that better because it's less action at a distance.

src/main.rs Outdated Show resolved Hide resolved
This object contains the actual status of `nixpkgs-check-by-name` checks.
@philiptaron philiptaron changed the title main: extract a status object main: extract a Status object to represent the final status of nixpkgs-check-by-name Jul 27, 2024
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Forgot about this, looking good, let's go!

@infinisil infinisil merged commit 9c7857e into NixOS:main Aug 22, 2024
3 checks passed
@philiptaron philiptaron deleted the misc branch August 22, 2024 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants