OsamaSayegh commented 4 years ago

Version 2.0.0 ships with optional rails patches, and the way to turn the patches on is to have this in your Gemfile: gem 'rack-mini-profiler', require: ['enable_rails_patches']. The enable_rails_patches.rb file defines a constant under Rack::MiniProfiler which means the Rack::MiniProfiler class will always be defined, even though Mini Profiler is not fully loaded.

This means if you have code that checks defined?(Rack::MiniProfiler) before calling methods on Rack::MiniProfiler and Mini Profiler hasn't fully loaded yet, your code will 💥.

I don't see any downsides to using ENV instead of constant here, what do you think @SamSaffron? If this is problematic, we can still use a constant but it would need to be directly under Rack (or a global constant, but I don't think that's desirable) e.g. Rack::MINI_PROFILER_ENABLE_RAILS_PATCHES.

codecov-io commented 4 years ago

Codecov Report

Merging #435 into master will not change coverage by %. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #435   +/-   ##
=======================================
  Coverage   88.53%   88.53%           
=======================================
  Files          22       22           
  Lines        1335     1335           
=======================================
  Hits         1182     1182           
  Misses        153      153

Impacted Files	Coverage Δ
lib/mini_profiler/profiler.rb	`84.31% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5985033...bef4ed7. Read the comment docs.

SamSaffron commented 4 years ago

Trouble with env is that it pollutes env in all forked processes.

I think putting this under Rack is ok for now, it seems like an alright compromise.

MiniProfiler / rack-mini-profiler

DEV: Use ENV hash instead of constant to enable rails patches #435

Codecov Report