Skip to content
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

History refactors #1013

Merged
merged 3 commits into from
Oct 11, 2024
Merged

History refactors #1013

merged 3 commits into from
Oct 11, 2024

Conversation

eval
Copy link
Contributor

@eval eval commented Oct 8, 2024

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_HISTORY and HISTORY_FILE.

The main change is that this PR bundles history logic in a separate module IRB::History, with helpers like save_history (IRB.conf[:SAVE_HISTORY] as an int), save_history?, infinite? and history_file. Logic that currently is duplicated and/or not explicitly named as such.
Furthermore, IRB::HistorySavingAbility.save_history is 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

  • permissions
    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

  • should conf.save_history. and conf.history_file be 'aliases' for resp. IRB::History.save_history and IRB::History.history_file?
    Pro: This way a user can more easily inspect what IRB will do, e.g. settings like IRB.conf[:SAVE_HISTORY] = nil are turned into conf.save_history #=> 0, and showing a clear path when IRB.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.

Copy link
Member

@tompng tompng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the pull request. The change looks basically great!

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|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code is more straightforward than calling history_file_present

history_file = History.history_file
return unless File.exist?(history_file)

history_file && Pathname.new(history_file).dirname.writable?
end

def append?(history_file, loaded_mtime)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

def folder_writable?
history_file && Pathname.new(history_file).dirname.writable?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this method can be concatenated into HistorySavingAbility#correct_permissions.
Similar logic in similar place is better.


# 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private

# Ensure existing +history_file+ is owner-only-readable [BUG #7694].
# Yields boolean whether chmod succeeded. `true` when file is absent.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# Yields boolean
Returns is better because Yields is confusing. There is a directive :yields: in RDoc comment that has another meaning.

@eval
Copy link
Contributor Author

eval commented Oct 10, 2024

Thanks for the feedback @tompng ! I made the changes.
I also added an extra commit that detaches loading the history from saving, allowing for readonly history - maybe it warrants a separate CLI-flag to ensure nothing happens to the history-file? I can move it to a separate issue/PR if you prefer.

Let me know if this one is finished and I'll rebase. Thanks again!

lib/irb.rb Outdated Show resolved Hide resolved
@eval eval force-pushed the history-refactors branch 2 times, most recently from 17f03a6 to 35864fc Compare October 10, 2024 14:39
Copy link
Member

@tompng tompng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good 👍
Thank you!

@tompng tompng merged commit 52307f9 into ruby:master Oct 11, 2024
30 checks passed
matzbot pushed a commit to ruby/ruby that referenced this pull request Oct 11, 2024
* Extract logic save_history in separate helper

* Extract logic history_file in helper

* Allow for readonly history

ruby/irb@52307f9026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants