-
Notifications
You must be signed in to change notification settings - Fork 370
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
refactor: just disable color output rather than tracking terminal width #1087
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1087 +/- ##
==========================================
- Coverage 65.40% 65.40% -0.01%
==========================================
Files 150 150
Lines 12518 12521 +3
==========================================
+ Hits 8188 8189 +1
- Misses 3872 3873 +1
- Partials 458 459 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Maybe it's because there multiple reporter.New() calls every run, and the disable color function is a single global variable. |
yeahhhhh but then why would it work for the vertical output...? something odd is def going on but I think it's probably best we deal with that once we've landed the vertical report since we plan to shuffle things around a bit re table anyway right |
Weirdly too it seems like it's a very stable global? I've run the test suite a ton and it never seems to give different results like I'd expect if this is an unstable use of a global...? (fwiw I vote we land this for now - we can easily revert if it turns out to be flakey) |
I mistakenly changed the order that styles are applied in #1087 for the table formatter so our custom styles were immediately overridden when the rounded style was applied
This lets us drop some conditionals - weirdly it doesn't seem to work if we move the conditional
text.DisableColors
call intoreporter.New
even though that does work for the vertical reporter...