Skip to content

fix hitory file path #1146

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

Merged
merged 1 commit into from
Jun 13, 2025
Merged

fix hitory file path #1146

merged 1 commit into from
Jun 13, 2025

Conversation

ko1
Copy link
Collaborator

@ko1 ko1 commented Jun 13, 2025

  • Set default of CONFIG[:history_file] as nil, and ignore it if it is nil.
  • Respect CONFIG[:history_file] even if the path doesn't exists.
  • Respect ~/.rdbg_history if exists for compatibility.
  • Use XDG_STATE_HOME if there is no configuration about history file.

Copy link

launchable-app bot commented Jun 13, 2025

Tests Failed

✖️no tests failed ✔️540 tests passed(140 flakes)

@ko1 ko1 force-pushed the fix_history_file_path branch from ad57d84 to f5012f9 Compare June 13, 2025 07:28
@ko1
Copy link
Collaborator Author

ko1 commented Jun 13, 2025

#1031

@ko1 ko1 closed this Jun 13, 2025
@ko1 ko1 reopened this Jun 13, 2025
@ko1 ko1 requested a review from Copilot June 13, 2025 08:24
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors how the history file path is determined, making the default configurable via CONFIG[:history_file], preserving legacy ~/.rdbg_history, and falling back to $XDG_STATE_HOME/rdbg/history.

  • Change default history_file config value from "~/.rdbg_history" to nil
  • Rewrite history_file method in console.rb to respect config, legacy path, then XDG state directory
  • Update README to document the new default path

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
lib/debug/console.rb Revised history_file logic to handle nil, legacy file, and XDG state path
lib/debug/config.rb Set default history_file to nil
README.md Updated default history_file documentation
Comments suppressed due to low confidence (1)

lib/debug/console.rb:153

  • Add tests for the new history_file logic: nil config, legacy ~/.rdbg_history, and XDG state directory fallback.
def history_file

* Set `CONFIG[:history_file]` as nil, and ignore it if it is nil.
* Respect `CONFIG[:history_file]` even if the path doesn't exists.
* Respect `~/.rdbg_history` if exists for compatibility.
* Use `XDG_STATE_HOME` if there is no configuration about history file.
@ko1 ko1 force-pushed the fix_history_file_path branch from f5012f9 to 4acb6bd Compare June 13, 2025 08:56
@ko1 ko1 merged commit 43f80ff into ruby:master Jun 13, 2025
27 of 32 checks passed
@jasonkarns
Copy link
Contributor

This is great! Do you think we need to document the fallback when XDG_STATE_HOME is unset? This PR does use the basedir specification's default (yay!), but that behavior isn't documented. Would it be welcome if I open a PR to modify the docs to that effect?

@ko1
Copy link
Collaborator Author

ko1 commented Jun 14, 2025

RUBY_DEBUG_HISTORY_FILE (history_file): history file (default: $XDG_STATE_HOME/rdbg/history)

is written. not enough?

@jasonkarns
Copy link
Contributor

jasonkarns commented Jun 17, 2025

I think the confusion is for users who have never heard of XDG before and don't have XDG_STATE_HOME env var set. They would have no idea where to find the file.

The code itself shows the fallback (~/.local/state) when XDG_STATE_HOME is unset, but that's not indicated in the comments/docs.

I would suggest using bash's own expansion syntax to indicate the env var's fallback in-line:

RUBY_DEBUG_HISTORY_FILE (history_file): history file (default: ${XDG_STATE_HOME-~/.local/state}/rdbg/history)

This way, they don't need to google XDG_STATE_HOME to find what its default value is.

Should I PR this?
PR open here #1148 so discussion can continue there instead of on this already-merged PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants