History refactors #1013
History refactors #1013
Conversation
lib/irb/history.rb
Outdated
| if history_file && File.exist?(history_file) | ||
| File.open(history_file, "r:#{IRB.conf[:LC_MESSAGES].encoding}") do |f| | ||
| if (history_file = History.history_file_present) | ||
| File.open(history_file, "r:#{History.file_encoding}") do |f| |
There was a problem hiding this comment.
IRB.conf[:LC_MESSAGES] is history's file_encoding, STDIN encoding and more.
It's something like a system encoding of IRB. I prefer leaving as IRB.conf[:LC_MESSAGES] instead of defining method only for history.
lib/irb/history.rb
Outdated
| history_file = IRB.rc_file("_history") unless history_file | ||
| if history_file && File.exist?(history_file) | ||
| File.open(history_file, "r:#{IRB.conf[:LC_MESSAGES].encoding}") do |f| | ||
| if (history_file = History.history_file_present) |
There was a problem hiding this comment.
I think this code is more straightforward than calling history_file_present
history_file = History.history_file
return unless File.exist?(history_file)
lib/irb/history.rb
Outdated
| history_file && Pathname.new(history_file).dirname.writable? | ||
| end | ||
|
|
||
| def append?(history_file, loaded_mtime) |
There was a problem hiding this comment.
I think the name is not so good. It feels like the logic in save_history is leaking to another module.
I think straightforward way like this is good enough.
if History.append?(history_file, @loaded_history_mtime)
# ↓
if File.exist?(history_file) && File.mtime(history_file) != @loaded_history_mtime
lib/irb/history.rb
Outdated
| end | ||
|
|
||
| def folder_writable? | ||
| history_file && Pathname.new(history_file).dirname.writable? |
There was a problem hiding this comment.
I think this method can be concatenated into HistorySavingAbility#correct_permissions.
Similar logic in similar place is better.
lib/irb/history.rb
Outdated
|
|
||
| # Ensure existing history_file is owner-only-readable [BUG #7694]. | ||
| # Yields boolean whether chmod succeeded. `true` when file is absent. | ||
| def correct_permissions!(history_file) |
There was a problem hiding this comment.
This module is included into InputMethod. The name InputMethod#correct_permissions! is not clear what it corrects.
How about a method name like correct_history_file_permissions history_file_writable! update_history_file_writable ensure_history_file_writable ?
Also, if the return value true means the file is writable with correct permission, I think this method should check folder permission too.
def new_name_of_correct_permissions!(history_file)
# Fail if folder is not writable (I think this method can contain this)
# Nothing to do if file does not exist
# update permission if file exists
end
lib/irb/history.rb
Outdated
| private | ||
|
|
||
| # Ensure existing history_file is owner-only-readable [BUG #7694]. | ||
| # Yields boolean whether chmod succeeded. `true` when file is absent. |
There was a problem hiding this comment.
# Yields boolean
Returns is better because Yields is confusing. There is a directive :yields: in RDoc comment that has another meaning.
|
Thanks for the feedback @tompng ! I made the changes. Let me know if this one is finished |
eval
force-pushed
the
history-refactors
branch
2 times, most recently
from
17f03a6
to
35864fc
Compare
* Extract logic save_history in separate helper * Extract logic history_file in helper * Allow for readonly history ruby/irb@52307f9026
As part of #1012 I started looking into history some more and came up with some refactors around saving/loading history and the settings
SAVE_HISTORYandHISTORY_FILE.The main change is that this PR bundles history logic in a separate module
IRB::History, with helpers likesave_history(IRB.conf[:SAVE_HISTORY]as an int),save_history?,infinite?andhistory_file. Logic that currently is duplicated and/or not explicitly named as such.Furthermore,
IRB::HistorySavingAbility.save_historyis currently quite complex in that there's multiple returns - these are now all in the top of the method with more descriptive names.Changes in functionality
Anything that might prevent the file from being saved (folder doesn't exist, lacking permissions to folder or file), now results in a warning.
Currently, there's only a warning when the folder does not exist. Lacking folder-permissions (to write a new file) will result in an exception. Lacking folder/file permissions for an existing file currently result in silently not saving.
I did quite some git spelunking to see what issues lay behind the current code, and tested locally different scenarios to ensure that nothing gets deleted.
The PR is split into 2 commits (save_history and history_file resp. changes) to aid reviewing. Let me know what's preferred for additional changes (additional commits or amending these commits, the latter can be noisy and hard to review).
(open) Issues
conf.save_history. andconf.history_filebe 'aliases' for resp.IRB::History.save_historyandIRB::History.history_file?Pro: This way a user can more easily inspect what IRB will do, e.g. settings like
IRB.conf[:SAVE_HISTORY] = nilare turned intoconf.save_history #=> 0, and showing a clear path whenIRB.conf[:HISTORY_FILE] = nil.Downside is that it might confuse as it's not what a user 'configged'.
PS I saw the hacktoberfest-label on the repo, this PR is not part of that, I'm not participating.