Add a command to delete history item at point

[Zzull]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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

[bbatsov]

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]

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

[bbatsov]

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]

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

[Zzull]

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]

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]

delete-item-at-point

[bbatsov]

(or entry)

[Zzull]

Done

[bbatsov]

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)

[Zzull]

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

[bbatsov]

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]

Frankly, I really care about history between sessions

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

[bbatsov]

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]

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]

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]

So, what are we doing about this PR?

[vemv]

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

[bbatsov]

@vemv Any updates on this front?

[vemv]

Perhaps this weekend, sorry for the delay.

[bbatsov]

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

[vemv]

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

[Zzull]

Is there any way I can help?

[vemv]

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

[bbatsov]

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

[Zzull]

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]

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

[Zzull]

Done

[bbatsov]

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

[Zzull]

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.

[bbatsov]

Fair enough.

[bbatsov]

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

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?

[Zzull]

Done

[Zzull]

• 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