-
Notifications
You must be signed in to change notification settings - Fork 64
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
Use $EDITOR as text editor fallback #184
Use $EDITOR as text editor fallback #184
Conversation
@@ -10,7 +10,7 @@ def git_root_dir | |||
end | |||
|
|||
def git_editor_command | |||
@git_editor_command ||= GitReflow::Config.get('core.editor') |
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.
Removed the memoize because GitReflow::Config#get
already seems to cache the "slow" code, and I got wrongly memoized values in the two it
blocks in the spec
Don't understand why the Circle tests suddenly fail after my last commit, which only removes an unneccesary ENV mock. Could someone with permissions on Circle trigger a rebuild without cache, please? |
@@ -10,7 +10,7 @@ def git_root_dir | |||
end | |||
|
|||
def git_editor_command | |||
@git_editor_command ||= GitReflow::Config.get('core.editor') | |||
GitReflow::Config.get('core.editor') || ENV['EDITOR'] |
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.
use GitReflow::DEFAULT_EDITOR
instead of ENV['EDITOR']
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.
Nice, didn't know that constant existed already :)
@@ -19,6 +19,23 @@ module Gitacular | |||
it { expect{ subject }.to have_run_command_silently "git rev-parse --show-toplevel" } | |||
end | |||
|
|||
describe '.git_editor_command' do | |||
subject { Gitacular.git_editor_command } | |||
before { allow(ENV).to receive(:[]).with('EDITOR').and_return 'vim' } |
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.
I would rather see this set explicitly to avoid issues this may pose with the CI: ENV['EDITOR'] = 'vim'
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.
done!
One last comment, and this LGTM. Thanks! |
I made GitReflow::DEFAULT_EDITOR a method instead of constant, because the constant caused mocking of ENV to be useless (the constant would be defined based on the actual ENV value BEFORE the mocking could took place) |
Failing spec is a one-second timing mismatch, rebuild should fix it. |
Awesome, nice work! 👍 |
If no editor is set up specifically for gitreflow, in its current state it would just skip editing the PR message when using
git-reflow review
, which I think is never desirable.This makes it fall back to
$EDITOR
which is set up on most unixy systems, and which I believe git also uses for commit messages