-
Notifications
You must be signed in to change notification settings - Fork 121
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
Allow loading multiple irb files #859
Conversation
Probably always at the user level? I can't think of any reason to keep a history file in the project folder, which users need to be careful not to commit 🤔
I think it's fine to separate them with just comma. The info is mainly for issue reporting purpose and we can always improve it later. |
f95e8fe
to
57f26ed
Compare
262026b
to
4885d71
Compare
09e4c67
to
d79a4b0
Compare
lib/irb/init.rb
Outdated
if !@CONF[:RC_NAME_GENERATOR] | ||
@CONF[:RC_NAME_GENERATOR] ||= [] | ||
existing_rc_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.
Since it's not actually a collection of files or file paths, but a collection of Procs, I think it should be called existing_rc_file_generators
instead. Otherwise the CONF[:RC_NAME_GENERATOR] = existing_rc_files
below looks a bit confusing.
lib/irb/init.rb
Outdated
break | ||
end | ||
@CONF[:RC_NAME_GENERATOR] << rcgen | ||
existing_rc_files << rcgen if File.exist?(rcgen.call(IRBRC_EXT)) |
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.
Should this be ext
instead? If it is, then we probably need more cases to cover the case where ext
is not rc
?
This allows hierarchy when loading rc files for example both files below are loaded; project/.irbrc ~/.irbrc Co-authored-by: Stan Lo <[email protected]>
d79a4b0
to
84668c8
Compare
@@ -11,7 +11,7 @@ def setup | |||
@backup_env = %w[HOME XDG_CONFIG_HOME IRBRC].each_with_object({}) do |env, hash| | |||
hash[env] = ENV.delete(env) | |||
end | |||
ENV["HOME"] = @tmpdir = Dir.mktmpdir("test_irb_init_#{$$}") | |||
ENV["HOME"] = @tmpdir = File.realpath(Dir.mktmpdir("test_irb_init_#{$$}")) |
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.
by using realpath I avoid the scenario where in tests I was getting files in the absolute or sym link path, this will resolve any symbolic links, example output of rcfiles which caused this issue:
<["/var/folders/mf/xxf7lzbx2ml7m2dx1dz8fw3c0000gn/T/test_irb_init_5716020240227-57160-q6cm7w/.irbrc",
"/private/var/folders/mf/xxf7lzbx2ml7m2dx1dz8fw3c0000gn/T/test_irb_init_5716020240227-57160-q6cm7w/.irbrc",
"/private/var/folders/mf/xxf7lzbx2ml7m2dx1dz8fw3c0000gn/T/test_irb_init_5716020240227-57160-q6cm7w/_irbrc",
"/private/var/folders/mf/xxf7lzbx2ml7m2dx1dz8fw3c0000gn/T/test_irb_init_5716020240227-57160-q6cm7w/$irbrc"]> was expected to include
<"/var/folders/mf/xxf7lzbx2ml7m2dx1dz8fw3c0000gn/T/test_irb_init_5716020240227-57160-q6cm7w/_irbrc">.
@st0012 I think I addressed all the comments, could you take another look please? |
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.
This will be a great improvement for IRB. Thank you!
(ruby/irb#859) This allows hierarchy when loading rc files for example both files below are loaded; project/.irbrc ~/.irbrc ruby/irb@b53ebc6655 Co-authored-by: Stan Lo <[email protected]>
Opening this PR for early feedback, issue: #674
Add the ability to load multiple irb files.
e.g. if user defines an irb file in
"/Users/user_name/.irbrc"
and inside the project"/Users/user_name/projects/ruby/irb/.irbrc"
both configs are loaded.rc_file
is deprecated with a deprecation notice, informing about a new method calledrc_files
to use instead.test_rc_file
andtest_rc_file_in_subdir
, not sure what we want to do with that, silence?todo