Add a command to delete history item at point #3574

Zzull · 2023-11-07T18:41:14Z

This PR adds a command to delete an history item at point.

The first commit is important because it makes cider-repl-input-history global. It was already sort of global before because the history was shared across REPLs and saved to a unique file but now it's downright a defvar instead of a defvar-local. I went in that direction because it felt like it was the simplest way of dealing with acknowledgment of deletion of an item across multiple REPLs.

The first commit now stores histories on a per-project basis

Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

The commits are consistent with our contribution guidelines
You've added tests (if possible) to cover your change(s)
All tests are passing (eldev test)
All code passes the linter (eldev lint) which is based on elisp-lint and includes
- byte-compilation, checkdoc, check-declare, packaging metadata, indentation, and trailing whitespace checks.
You've updated the changelog (if adding/changing user-visible functionality)
You've updated the user manual (if adding/changing user-visible functionality)

Thanks!

If you're just starting out to hack on CIDER you might find this section of its
manual extremely useful.

Zzull · 2023-11-07T18:50:19Z

On a side note, it looks like eldev linter doesn't agree with the CI on how to indent

cider-repl.el:354:0 (indent)     !          (contents)
cider-repl.el:355:0 (indent)     !          (insert-before-markers
cider-repl.el:356:0 (indent)     !           (propertize
cider-repl.el:357:0 (indent)     !            (if (string-blank-p contents) ";;\n" (concat ";; " contents "\n"))
cider-repl.el:358:0 (indent)     !            'font-lock-face 'font-lock-comment-face))))
Found 5 warnings in file `cider-repl.el'

vemv · 2023-11-07T18:53:25Z

Thanks for the PR!

The first commit is important because it makes cider-repl-input-history global. It was already sort of global before because the history was shared across REPLs

Although it may be true, I sense that having per-project repl histories would be the best new direction - WDYT?

vemv · 2023-11-07T18:53:57Z

On a side note, it looks like eldev linter doesn't agree with the CI on how to indent

Probably it's a matter of using our Makefile instead

Zzull · 2023-11-07T19:34:36Z

On a side note, it looks like eldev linter doesn't agree with the CI on how to indent

Probably it's a matter of using our Makefile instead

make lint and eldev lint yielded the same result (that cider-repl.el has a block poorly indented)

Zzull · 2023-11-07T19:36:20Z

Thanks for the PR!

The first commit is important because it makes cider-repl-input-history global. It was already sort of global before because the history was shared across REPLs

Although it may be true, I sense that having per-project repl histories would be the best new direction - WDYT?

I can totally try to split the history on a per-project basis

vemv · 2023-11-07T20:46:31Z

make lint and eldev lint yielded the same result (that cider-repl.el has a block poorly indented)

Maybe you're on a different Emacs version - we use 28 for linting. The Makefile has a comment indicating how to override the local Emacs.

I can totally try to split the history on a per-project basis

Thanks!

I assume it's never been like that / it's something we want? @bbatsov

To me it makes sense because a given project's sexprs are often irrelevant / invalid (not compile-able) in another project.

Zzull · 2023-11-07T23:28:34Z

Maybe you're on a different Emacs version - we use 28 for linting. The Makefile has a comment indicating how to override the local Emacs.

Absolutely. Thank you

To me it makes sense because a given project's sexprs are often irrelevant / invalid (not compile-able) in another project.

It's done in the first commit (which is a bit hard to read…)

bbatsov · 2023-11-08T06:09:03Z

I assume it's never been like that / it's something we want? @bbatsov

I'd be fine by this, although I'm not sure how it can be made reliably.(e.g. what would you use as project id for remote connections?) I'll take a look at the proposed implementation.

bbatsov · 2023-11-08T06:13:14Z

cider-repl.el

+The value of `cider-repl-input-history' and `cider-repl-history-file' are set by
+this function."
+  (when-let* ((dir (clojure-project-dir))
+              (file (concat dir "/.cider-history")))


I'd better to use expand-file-name instead of concat. Also - I'd move the file name to a constant.

bbatsov · 2023-11-08T06:14:24Z

cider-repl.el

@@ -1592,16 +1595,6 @@ If USE-CURRENT-INPUT is non-nil, use the current input."
  :type 'integer
  :safe #'integerp)

-(defcustom cider-repl-history-file nil


You can't just delete this - you'll have to add a deprecation warning for it, so people would know it's no longer being used. If we want to avoid breaking changes we'd probably want to keep the old behavior if this variable is set.

bbatsov · 2023-11-08T06:16:15Z

cider-repl.el

@@ -194,6 +194,9 @@ CIDER 1.7."
 This property value must be unique to avoid having adjacent inputs be
 joined together.")

+(defvar-local cider-repl-history-file nil


Related to my other remark - you should probably use a different name for the buffer-local var, to avoid breaking changes.

bbatsov · 2023-11-08T06:18:53Z

I quickly went over the code and it looks reasonable overall, but it also introduces a breaking change that I think would be prudent to ignore. Probably it'd be best to keep the old behavior if someone set an file name explicitly and switch to the logging per project if they haven't.

vemv · 2023-11-08T09:03:11Z

cider-repl.el

+The value of `cider-repl-input-history' and `cider-repl-history-file' are set by
+this function."
+  (when-let* ((dir (clojure-project-dir))
+              (file (concat dir "/.cider-history")))


Probably we should have one file per platform: clj, cljs, bb

cider-repl.el

Zzull · 2023-11-10T14:50:08Z

Hello,

Almost all comments have been addressed.

I still have two things I would like your advice on:

I don't have a remote machine accessible with clojure on it to make sure that cider-repl--find-dir-for-history behaves the way it should. Do you think tramp-tramp-file-p is enough?
I couldn't find a way to get the type of the REPL to suffix the name of the history file with. It seems that it is too early to call (cider-runtime) (returns 'generic) nor get the value of cider-repl-type (nil) when cider-repl-history-load is called. And anyways, after initialization, (cider-runtime) returns 'clojure for a cljs project and cider-repl-type has the value 'clj for a babashka project. Do you have other leads?

vemv · 2023-11-10T21:47:11Z

Thanks much!

It's looking great.

Do you think tramp-tramp-file-p is enough?

Most definitely.

Do you have other leads?

If you don't mind I'll pick up this branch, fix some nits and then also add the 'platform' refinement. Then I'll squash everything, keeping you as the author. It's how I operate normally, to avoid long feedback loops.

We can keep the PR open in the meantime.

bbatsov · 2023-11-12T16:49:53Z

cider-repl-history.el

@@ -576,6 +577,16 @@ text from the *cider-repl-history* buffer."
  (with-current-buffer cider-repl-history-repl-buffer
    (undo)))

+(defun cider-repl-history-delete ()


delete-item-at-point

bbatsov · 2023-11-12T16:54:13Z

doc/modules/ROOT/pages/repl/configuration.adoc

-Note that CIDER writes the history to the file when you kill the REPL
-buffer, which includes invoking `cider-quit`, or when you quit Emacs.
+Note that CIDER stores the history in a file named by
+`cider-repl-local-history-name` located at the root of your project if


Probably this should be named cider-repl-project-history-file or something along those lines, as I think local history might be slightly misleading in this case. (and yeah - I realize there might not always be a project associated with a REPL)

I changed the implementation quite a bit and removed the notion of global and local

bbatsov · 2023-11-12T16:55:33Z

I'll also add that currently CIDER doesn't have persistent history by default and I think it's a good idea to keep this behavior. Frankly, I really don't care much about history between sessions and I guess I'm not the only one, given there has been little demand to change the default behavior over the years.

vemv · 2023-11-12T16:59:56Z

Frankly, I really care about history between sessions

I didn't get this - what behavior should remain unchanged?

bbatsov · 2023-11-12T17:03:05Z

I meant to write "don't care". I think this should be some opt-in, not a default. (just as storing the global history to file is)

vemv · 2023-11-12T17:06:03Z

From my side it's something I've always missed (really), but it was more in the 'nice-to-have' area. There was always something more important 😄

Maybe we can keep it under a defcustom indeed, enabling it later after some months for gaining confidence in the solution.

bbatsov · 2023-11-12T17:43:17Z

Yeah, that's fair. I'm just always a bit wary of serializing data to files (and reading it back) as often people run into issues with broken entries due to various reason. (probably less so in this case, given the simple nature of the history format, but I've had my fair share of issues with seemingly trivial serialization)

bbatsov · 2023-11-27T12:03:45Z

So, what are we doing about this PR?

vemv · 2023-11-27T12:37:53Z

I'll pick it up early December, sorry for the delay.

bbatsov · 2024-02-28T12:34:05Z

@vemv Any updates on this front?

vemv · 2024-02-28T12:35:09Z

Perhaps this weekend, sorry for the delay.

bbatsov · 2024-05-07T12:21:35Z

@vemv Friendly reminder about this one. :-) I want us to start finalizing the scope of the next release.

vemv · 2024-05-07T12:24:46Z

Sure, I'm finally closing a sprint which didn't leave a lot of OSS time available.

Zzull · 2024-11-15T10:17:57Z

Is there any way I can help?

vemv · 2024-11-15T10:24:16Z

I can't allocate time currently so you can go ahead in any way that works / passes review from @bbatsov

bbatsov · 2024-11-15T13:10:13Z

@Zzull Just address my previous comments and we're good to go.

Zzull · 2024-11-18T17:45:20Z

I'll also add that currently CIDER doesn't have persistent history by default and I think it's a good idea to keep this behavior. Frankly, I really don't care much about history between sessions and I guess I'm not the only one, given there has been little demand to change the default behavior over the years.

I changed the implementation so that CIDER stays without a persistent history by default.
The documentation of cider-repl-history-file went from:

"File to save the persistent REPL history to.
If this is set, the history will be global to all projects.  Otherwise, the
history is local per project and stored in a file (named by
`cider-repl-local-history-name') at its root."

to

"File to save the persistent REPL history to.
If this is set to a path the history will be global to all projects.  If this is
set to `per-project', the history will be stored in a file (named by
`cider-repl-project-history-name') at the root of each project."

I still haven't found a way to get the REPL type though. I described it earlier:

Hello,
…
* I couldn't find a way to get the type of the REPL to suffix the name of the history file with. It seems that it is too early to call (cider-runtime) (returns 'generic) nor get the value of cider-repl-type (nil) when cider-repl-history-load is called. And anyways, after initialization, (cider-runtime) returns 'clojure for a cljs project and cider-repl-type has the value 'clj for a babashka project. Do you have other leads?

bbatsov · 2024-12-08T07:03:09Z

CHANGELOG.md

@@ -2,10 +2,14 @@

 ## master (unreleased)

+### New features


You'll need to rebase, as we cut a new CIDER release recently.

Sorry about the slow responses here - I had a couple of busy weeks, but I'll use the holidays to catch up with oss work and finally merge your PR.

bbatsov · 2024-12-08T07:09:57Z

cider-repl.el

+(defun cider-repl--find-dir-for-history ()
+  "Find the first suitable directory to store the project's history."
+  (seq-find
+   (lambda (dir) (and (not (null dir)) (not (tramp-tramp-file-p dir))))


Is there an issue with storing the history on a remote filesystem?

I actually don't know and I don't have access to a remote machine on which I can start CIDER on to try. I followed @vemv advice:

It may be better to pick the first, non-nil, non-tramp value of (list nrepl-project-dir clojure-project-dir default-directory)

This way we don't write files to remote TRAMP targets, which may surprise most people - or bring unforeseen issues.

Fair enough.

bbatsov · 2024-12-08T07:12:57Z

I couldn't find a way to get the type of the REPL to suffix the name of the history file with. It seems that it is too early to call (cider-runtime) (returns 'generic) nor get the value of cider-repl-type (nil) when cider-repl-history-load is called. And anyways, after initialization, (cider-runtime) returns 'clojure for a cljs project and cider-repl-type has the value 'clj for a babashka project. Do you have other leads?

There's not an easy way around this, so I guess we can skip it for the first version. I think the per-project history will be quite the improvement, even without being so granular.

bbatsov · 2024-12-08T07:15:22Z

doc/modules/ROOT/pages/repl/configuration.adoc


 [source,lisp]
 ----
 (setq cider-repl-history-file "path/to/file")
 ----

-Note that CIDER writes the history to the file when you kill the REPL
-buffer, which includes invoking `cider-quit`, or when you quit Emacs.
+Note that CIDER stores the history in a file named by


I'd probably expand this to show a full configuration snippet, so it's clearer how people are supposed to set this up. Perhaps you can just have a bullet point for the project-specific configuration so it stands out a bit more?

To save the history on a per-project basis.

Because input history cannot be changed by another REPL anymore

Zzull · 2024-12-10T11:40:39Z

I couldn't find a way to get the type of the REPL to suffix the name of the history file with. It seems that it is too early to call (cider-runtime) (returns 'generic) nor get the value of cider-repl-type (nil) when cider-repl-history-load is called. And anyways, after initialization, (cider-runtime) returns 'clojure for a cljs project and cider-repl-type has the value 'clj for a babashka project. Do you have other leads?

There's not an easy way around this, so I guess we can skip it for the first version. I think the per-project history will be quite the improvement, even without being so granular.

I removed the commit introducing that change

bbatsov · 2024-12-23T14:45:32Z

@Zzull Can you also rebase on master to resolve the Changelog conflict?

Zzull force-pushed the master branch from 225e45d to e98aabf Compare November 7, 2023 18:47

Zzull force-pushed the master branch 2 times, most recently from 995f11b to d96930b Compare November 7, 2023 23:25

Zzull force-pushed the master branch from d96930b to 47608c1 Compare November 7, 2023 23:58

bbatsov reviewed Nov 8, 2023

View reviewed changes

vemv reviewed Nov 8, 2023

View reviewed changes

Zzull force-pushed the master branch from 47608c1 to 46bf16a Compare November 10, 2023 14:34

Zzull force-pushed the master branch 2 times, most recently from 106f764 to 41cccdf Compare November 10, 2023 15:05

vemv marked this pull request as draft November 11, 2023 22:47

bbatsov reviewed Nov 12, 2023

View reviewed changes

bbatsov assigned vemv Feb 28, 2024

Zzull force-pushed the master branch from 41cccdf to c8732ed Compare November 18, 2024 17:45

Zzull marked this pull request as ready for review November 18, 2024 17:46

Zzull force-pushed the master branch 2 times, most recently from c539731 to 3c6780f Compare November 21, 2024 15:46

bbatsov reviewed Dec 8, 2024

View reviewed changes

Zzull added 5 commits December 10, 2024 11:59

New value per-project for cider-repl-history-file

53a0457

To save the history on a per-project basis.

Stop merging the input history with `cider-repl-history-file'

9c10789

Because input history cannot be changed by another REPL anymore

Find a suitable directory to store the project's history more robustly

7183c1e

Calculate the length of the history only once

7c998c2

Add a command to delete history item at point

8a0c64e

Zzull force-pushed the master branch from 3c6780f to 8a0c64e Compare December 10, 2024 11:37

bbatsov approved these changes Dec 23, 2024

View reviewed changes

Add a command to delete history item at point #3574

Are you sure you want to change the base?

Add a command to delete history item at point #3574

Conversation

Zzull commented Nov 7, 2023 • edited Loading

Zzull commented Nov 7, 2023

vemv commented Nov 7, 2023 • edited Loading

vemv commented Nov 7, 2023

Zzull commented Nov 7, 2023

Zzull commented Nov 7, 2023

vemv commented Nov 7, 2023

Zzull commented Nov 7, 2023

bbatsov commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Nov 8, 2023

Choose a reason for hiding this comment

Zzull commented Nov 10, 2023 • edited Loading

vemv commented Nov 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Nov 12, 2023 • edited Loading

vemv commented Nov 12, 2023

bbatsov commented Nov 12, 2023

vemv commented Nov 12, 2023

bbatsov commented Nov 12, 2023

bbatsov commented Nov 27, 2023

vemv commented Nov 27, 2023 • edited Loading

bbatsov commented Feb 28, 2024

vemv commented Feb 28, 2024

bbatsov commented May 7, 2024

vemv commented May 7, 2024

Zzull commented Nov 15, 2024

vemv commented Nov 15, 2024

bbatsov commented Nov 15, 2024

Zzull commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Dec 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zzull commented Dec 10, 2024

bbatsov commented Dec 23, 2024

Zzull commented Nov 7, 2023 •

edited

Loading

vemv commented Nov 7, 2023 •

edited

Loading

Zzull commented Nov 10, 2023 •

edited

Loading

bbatsov commented Nov 12, 2023 •

edited

Loading

vemv commented Nov 27, 2023 •

edited

Loading