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

Various improvements #8

Merged
merged 1 commit into from Nov 5, 2022
Merged

Various improvements #8

merged 1 commit into from Nov 5, 2022

Conversation

Aloso
Copy link
Contributor

@Aloso Aloso commented Nov 5, 2022

This PR removes some string allocations in the supports_color function.

I originally wanted to use Result::as_deref for this, but since I wasn't sure what the minimum supported Rust version is, and Result::as_deref is only available since 1.47, I just wrote a helper function instead.

Other changes:

  • I removed min since it is guaranteed to be 0 everywhere where it is used
  • I reordered some code paths to do less work by returning earlier if FORCE_COLOR or NO_COLOR is set
  • I fixed the tests, which were flaky; cargo runs tests in parallel, which does not work properly if multiple tests read and write environment variables. The solution was to add a lock that ensures the tests run sequentially.

Copy link
Owner

@zkat zkat 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 awesome, and a welcome change! Thanks a lot!

src/lib.rs Show resolved Hide resolved
@zkat zkat merged commit ad90fe9 into zkat:main Nov 5, 2022
@zkat
Copy link
Owner

zkat commented Nov 5, 2022

published as supports-color@1.3.1 :)

@Aloso Aloso deleted the improvements-by-lu branch November 7, 2022 11:03
@Aloso
Copy link
Contributor Author

Aloso commented Nov 7, 2022

Thank you!

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.

None yet

2 participants