Save REPL history to .repl_history in .cache/roc

[hristog]

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 where cache_dir is referenced). Essentially, all cache_dir references become cache_packages_dir (or packages_cache_dir), as applicable, and cache_packages_dir is a function which itself calls cache_dir and joins packages 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.

[hristog]

I've pushed a commit "fixing" the multiline_string_non_wasm test (i.e., making it work), but I have to admit the value looks a bit odd (as in, the first character is missing). I'm afraid my Rust/rustyline knowledge isn't at a sufficiently good level to be able to confidently know whether or not this behaviour is indeed expected. If it is, then the test should probably be updated in a way, so that the expected value itself is a bit more intuitive. However, I strongly suspect that we don't want to be including the history message into any kind of doc comment responses (which the test suite suggests that at least one corresponding test has been affected).

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 "No previous history loaded." instead of "History loaded successfully..

I hope this makes sense.

[Anton-4]

I've opened #6616 for the packages dir issue.

[Anton-4]

it also seems to me that we probably don't want to be preserving REPL history across tests

That's a great idea! Could you add a --no-history flag for the repl command?

[hristog]

it also seems to me that we probably don't want to be preserving REPL history across tests

That's a great idea! Could you add a --no-history flag for the repl command?

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 .cache dir issue, as well as the described situation with the unit-tests, as I strongly believe, as it currently stands - this PR should definitely not be merged prior to either of these two points having been sorted out firstly. Correspondingly, I'll mark this PR as a draft, so that this is more clearly indicated in the meantime.

[github-actions[bot]]

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 Closed are not deleted, so no matter what, the PR will still be right here in the repo. You can always access it and reopen it anytime you like!