-
-
Notifications
You must be signed in to change notification settings - Fork 270
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
Save REPL history to .repl_history in .cache/roc #6593
Conversation
I've pushed a commit "fixing" the In the context of unit-tests, it also seems to me that we probably don't want to be preserving REPL history across tests, except for tests where this functionality is deliberately under test. In other words, in a well-isolated test-suite, I'd expect to output like I hope this makes sense. |
I've opened #6616 for the packages dir issue. |
Signed-off-by: Anton-4 <17049058+Anton-4@users.noreply.github.com>
That's a great idea! Could you add a |
Yes, sure! That'd be definitely useful indeed. I'll clarify what I meant in the quoted excerpt - I was referring to the unit-tests themselves and that we probably want the execution of each one to be independent from others, i.e., we don't want to store execution history across tests, unless that's explicitly what we want to test. I've had a TODO note (for a while, but haven't gotten the chance to get to it yet) to work on the |
Thank you for your contribution! Sometimes PRs end up staying open for a long time without activity, which can make the list of open PRs get long and time-consuming to review. To keep things manageable for reviewers, this bot automatically closes PRs that haven’t had activity in 60 days. This PR hasn’t had activity in 30 days, so it will be automatically closed if there is no more activity in the next 30 days. Keep in mind that PRs marked |
Description
tl;dr This PR proposes utilising
rustyline
's history loading and saving capabilities to enable preserving Roc REPL history across sessions (e.g., in the event of a REPL crash).This PR doesn't propose any way of controlling the history size and that is solely depending on
rustyline
's implementation.More context in this ZulipChat thread. As of the thread creation time, I hadn't checked the corresponding part of the codebase in detail and wasn't aware that
rustyline
already provides quite a bit of useful functionality in this regard.Discussion
.cache/roc
As it currently stands, crates/packaging/src/cache.rs defines the cache directory to be the
.cache/roc/packages
directory, instead of the.cache/roc/
directory. Correspondingly, the REPL history file becomes.cache/roc/packages/.repl_history
.I'm not sure if it fits within the scope of this PR (to me it feels it doesn't), but I would like to propose that the cache directory gets defined as
.cache/roc/
(and the Windows analogue) instead, and a new packages function, dependent on the root cache directory function is defined to be utilised in the respective places (as of this point in time, it seems everywhere wherecache_dir
is referenced). Essentially, allcache_dir
references becomecache_packages_dir
(orpackages_cache_dir
), as applicable, andcache_packages_dir
is a function which itself callscache_dir
and joinspackages
to the root cache directory path.If this proposal - for re-factoring references to the cache directories (i.e., root vs
packages
) - is going to be introduced, then this PR should also be kept into consideration, in terms of:(i) ensuring that the updated references are to the root cache directory;
(ii) that there is some sort of an indication to the users that the REPL history cache has moved from
packages
to root (in.cache/roc
and its Windows equivalent).I made several attempts to utilise
.parent()
(in a readable way) to refer to the root cache directory, but couldn't get that to work, i.e., compile (I'm still quite new to Rust). If somebody would like to give a hand with it, in the context of this PR, it would be greatly appreciated. It will render (ii) as irrelevant (which is good), but (i) will still need to be kept in mind.WASM platform
Another question which arises is - regarding WASM, it seems that the cache directory is
.cache/roc
(.cache/roc/packages
). Does this have any implications on the current PR? I suppose not, but double-checking just in case.