search

thekompanee / fuubar

The instafailing RSpec progress bar formatter
http://jeffkreeftmeijer.com/2010/fuubar-the-instafailing-rspec-progress-bar-formatter/
MIT License
955 stars 65 forks source link

Disable fuubar_auto_refresh by default #109

Closed davidstosik closed 5 years ago

davidstosik commented 5 years ago

Fixes #108.

_Initially, this PR meant to improve the workaround for fuubar_auto_refresh not to bother Pry. After discussing with @jfelchner (below here), it looks like a saner option is just to keep fuubar_auto_refresh disabled by default and let the user enable it. See original description in the hidden block below._

Stop auto refresh when a Pry session starts The problem with #105 was that the Pry-related code was running as soon as the `fuubar` gem got loaded, which can be before `Pry` is defined (if the `pry` gem gets loaded later). Why This Change Is Necessary -------------------------------------------------------------------------------- - [x] Bug Fix (see #108) - [ ] New Feature How These Changes Address the Issue -------------------------------------------------------------------------------- In the changes I propose here, - the Pry hooks are added later in the process, only when Fuubar starts, which allows: - making sure all the gems in the Gemfile were loaded before testing the presence of `Pry` - avoiding to add Pry hooks if the Fuubar output is not used (eg. I usually use Fuubar, but sometimes run `rspec --format documentation`) - I used :before_session and :after_session hooks, allowing Fuubar to keep refreshing as long as no Pry session is started Side Effects Caused By This Change -------------------------------------------------------------------------------- - [ ] This Causes a Breaking Change - [x] This Does Not Cause Any Known Side Effects Not a side-effect, but a know limitation: if the `pry` gem is added to the Gemfile with the option `require: false`, then required too late for Fuubar to know, then the hooks will not be added, and auto-refresh will keep happening when the Pry session opens. For example: ```rb # in Gemfile gem "pry", require: false gem "pry-byebug", require: false # in spec/whatever_spec.rb it "works" do # test something... require "pry-byebug" binding.pry end ``` Checklist: -------------------------------------------------------------------------------- - [ ] I have run `rubocop` against the codebase There seems to be an issue with Rubocop: > Error: invalid EnforcedStyle 'normal' for Layout/EmptyLinesAroundAccessModifier found in .rubocop_core.yml > Valid choices are: around, only_before - [ ] I have added tests to cover my changes There are no tests related to Pry. I would like to add some, but don't know how to start. Happy to add some if I can get help getting started. - [ ] All new and existing tests passed
jfelchner commented 5 years ago

@davidstosik thanks for the PR but to be honest I don't like having any of this stuff relating to other gems in the code. If you use a debugger workflow, it'd probably be best just to turn off auto-refresh in one of your spec support files.

What I think I'm going to do is to remove the Pry code and then set auto_refresh to false and make it an "opt in" situation. As much as I like auto-refresh, I don't want to break people's workflows.

davidstosik commented 5 years ago

Thank you @jfelchner. I think making auto_refresh an opt-in behavior is a nice alternative.

davidstosik commented 5 years ago

@jfelchner I changed this PR approach to disable fuubar_auto_refresh by default, as discussed above, and updated the documentation as well. Please let me know what you think! 🙏

jfelchner commented 5 years ago

@davidstosik this is fantastic. Thank you. 💖

foton commented 5 years ago

Nice solution and documentation. Thanks.

geoguide commented 5 years ago

Is this going to get merged? Alternatively is there a way to disable auto_refresh when using the command line?

jfelchner commented 5 years ago

My bad everyone. I completely forgot about this. I'll get this in this weekend.

kfrz commented 5 years ago

:clap: Nice one @davidstosik - look forward to having this merged!

The CI is failing for ruby-2.5.0, complaining it can't find bundler, other 2.0+ rubies are :green_heart:!

jfelchner commented 5 years ago

Merged in d9304a4

github-actions[bot] commented 3 years ago

This issue has been closed automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.