-
Notifications
You must be signed in to change notification settings - Fork 115
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
Fix irb_history saved to current directory #901
Conversation
Also split irbrc search logic from irb_history search logic as a refactor
This conf is used to specify which irbrc to load. Need to configure before irbrc is loaded, so it's actually not configurable. This conf is also used for history file search, but it is configurable by conf[:HISTORY_FILE].
test/irb/test_init.rb
Outdated
assert_equal(tmpdir+"/.irb_history", IRB.rc_file("_history")) | ||
reset_rc_name_generators | ||
assert_empty(IRB.irbrc_files) | ||
assert_equal("#{ENV["HOME"]}/.irb_history", IRB.rc_file("_history")) |
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.
Isn't this the same as tmpdir
? I understand that it's to emphasise that this value should be home, but I feel it could be misleading if we use ENV["HOME"]
and tmpdir
for the same value.
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'll remove this test_rc_file
because history file is tested more in another test test_rc_files
lib/irb/init.rb
Outdated
if !@CONF[:RC_NAME_GENERATOR] | ||
@CONF[:RC_NAME_GENERATOR] ||= [] | ||
existing_rc_file_generators = [] | ||
def IRB.prepare_rc_name_generators |
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.
Maybe we can declare this as private?
Also, I feel prepare_irbrc_name_generators
is a clearer name.
lib/irb/init.rb
Outdated
end | ||
|
||
# possible irbrc files in current directory | ||
def IRB.generate_current_dir_irbrc_files |
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.
Let's make this private too.
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.
Also changed rc_file_generators to private too
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.
Technically this is a breaking change as it removes RC_NAME_GENERATOR
from IRB.conf
entries. But from the search on GH, most (if not all) usages of it should have been using ENV["IRBRC"]
instead. The number of its usages also looks small (maybe a few dozens?).
Therefore, I think we can bite the bullet and remove it to make things simpler.
(ruby/irb#901) * Always save irb_history in HOME or XDG_CONFIG_HOME Also split irbrc search logic from irb_history search logic as a refactor * Remove IRB.conf[:RC_NAME_GENERATOR] because it's not configurable This conf is used to specify which irbrc to load. Need to configure before irbrc is loaded, so it's actually not configurable. This conf is also used for history file search, but it is configurable by conf[:HISTORY_FILE]. * remove rc_file_test because it is tested with rc_files, remove useless test setup * Make internal irbrc searching method private ruby/irb@11d03a6ff7
I think |
(ruby/irb#901) * Always save irb_history in HOME or XDG_CONFIG_HOME Also split irbrc search logic from irb_history search logic as a refactor * Remove IRB.conf[:RC_NAME_GENERATOR] because it's not configurable This conf is used to specify which irbrc to load. Need to configure before irbrc is loaded, so it's actually not configurable. This conf is also used for history file search, but it is configurable by conf[:HISTORY_FILE]. * remove rc_file_test because it is tested with rc_files, remove useless test setup * Make internal irbrc searching method private ruby/irb@11d03a6ff7
Fixes #674 with refactoring
IRB.rc_files is used for irbrc searching and irb_history searching. I think two separate logic is combined into one and making the code complicated.
Changes
IRB.rc_files(ext)
toIRB.irb_files
andIRB.rc_file(ext)
dirname(ENV['IRBRC'])
,ENV['XDG_CONFIG_HOME']
orENV['HOME']
IRB.conf[:RC_NAME_GENERATOR]
is removed because it's impossible to configure before loading irbrc.irbrc
's loading behaviour #674 (comment)