-
-
Notifications
You must be signed in to change notification settings - Fork 152
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
DRAFT: refactor: COLOR_MODE to OnceCell #365
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #365 +/- ##
==========================================
+ Coverage 63.42% 64.20% +0.78%
==========================================
Files 25 25
Lines 1572 1573 +1
==========================================
+ Hits 997 1010 +13
+ Misses 575 563 -12 ☔ View full report in Codecov by Sentry. |
Looks like CI is failing on this one still... |
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.
So, I think a better approach might be to continue to use once_cell::Lazy
that way each callsite remains really lightweight. But with the twist that you access it through a getter like pub fn get_color_mode()
that has some #[cfg(test)]
s that allow you to supplant the normal logic.
Does that make sense?
I see what you're saying but I don't think we get any test benefit with that change (which may be okay because of the integration tests). That said, I don't think I agree that using |
@bconn98 by lightweight i mean not having the following copy pasted at every callsite. There are other options if you prefer, you could wrap the Lazy in a struct and in cfg(test) mode internally it could simply be a Mutex that you can mutate as necessary.
|
I tried a couple different versions of using mutex.
But no avail, the Lazy will not execute a second time thereby causing tests to fail. I wouldn't want to put the actual ENV values inside of mutex's as that would invalidate the majority of the test. |
…rt test driven dependency injection
I found a solution that I think is satisfactory. By |
Refactor COLOR_MODE to be easier for testing and for using features in the standard library come v1.71.0.
Addresses: #368