climate_control icon indicating copy to clipboard operation
climate_control copied to clipboard

"ClimateControl.env is deprecated. ENV should be used instead"

Open dorianmariecom opened this issue 2 years ago • 2 comments

cc: @JoshCheek

dorianmariecom avatar May 28 '22 06:05 dorianmariecom

My opinion is the method is fine and the other code should use it instead of directly referencing the global constant. I generally like that better because I don't like code directly modifying global state, so adding the method in the middle makes it slightly less direct (eg could then be mixed in and overridden). However, I get that not everyone will agree with me, and removing the method will also make it consistent.

🤔 I guess there could be a difference in understanding, so I'll clarify that if the code wants to explicitly mutate ENV, that one singleton constant, then there is no need for this method. But I interpreted that method more like Capybara's app, whatever it returns is what will be mutated, since the obvious case is to modify ENV, that's what it returns.

JoshCheek avatar May 29 '22 05:05 JoshCheek

I feel like it would be an indirect use of a global variable (hiding this fact doesn't make it better :) ).

The fact is that the best compromise is to use the global ENV (I feel replacing / wrapping ENV would be a pain).

Also people could depend either on ClimateControl.env or ENV so better have only one public interface and not mislead people into thinking using ClimateControl.env will be different than relying on ENV.

So I'm kind of leaning towards depreciation then removal.

dorianmariecom avatar May 30 '22 06:05 dorianmariecom