search

clojure-emacs / inf-clojure

Basic interaction with a Clojure subprocess
249 stars 46 forks source link

All strings sent to REPL are converted from multi-line to single-line #187

Closed RobertARandolph closed 3 years ago

RobertARandolph commented 3 years ago

Expected behavior

Text sent to REPL is sent as-is, without conversion or parsing.

I view this as expected behaviour given that this is traditionally how inferior-lisp works, and the standard clojure REPL supports multiline strings without issue.

Actual behavior

inf-clojure converts all strings to a single-line before sending to the comint process.

This causes the 1024 TTY character limit per line problem on macOS to be frequently encountered in multiple scenarios.

There are at least 3 related issues to this line of code:

https://github.com/clojure-emacs/inf-clojure/issues/152 https://github.com/clojure-emacs/inf-clojure/issues/40 https://github.com/clojure-emacs/inf-clojure/issues/183

and a branch with a (strange) potential fix:

https://github.com/clojure-emacs/inf-clojure/tree/fix-152

Steps to reproduce the problem

Unnecessary.

Other

I've been using inf-clojure with the referenced code (and associated calls) removed for 2 weeks full-time without issue in clj and cljs repls. I used babashka for a short period of time without issue as well.

I have not tried planck/lumo/joker.

bbatsov commented 3 years ago

It took me a while, but I found the original problem - https://github.com/clojure-emacs/inf-clojure/issues/7

arichiardi commented 3 years ago

If memory serves me well lumo is the one that caused problems. The fix looks good to me.

Edit, lol sorry I haven't had time to finish that up

bbatsov commented 3 years ago

No worries. I had forgotten about that patch, but I see that it bring back the subprompt issue in certain cases. If it only impacts Lumo, I guess that's fine - from what I know it's pretty much dead these days.

dpsutton commented 3 years ago

i believe it is officially deprecated. i suppose if we wanted to we could make this an option, automatically on for lumo, or just drop it entirely.

arichiardi commented 3 years ago

Agree we should proceed with the patch and special case for lumo maybe, in order not to break people...my 2c

RobertARandolph commented 3 years ago

Has it been tried with Lumo? I can't even get Lumo to run on my machine.

arichiardi commented 3 years ago

I tried it when I wrote my last comment in the PR. An if on the repl type should be enough. The other option is breaking, which probably is ok as long as we write this in the Changelog (or maybe Bozhidar has a better idea)

bbatsov commented 3 years ago

Well, let's merge your patch and see what happens.

RobertARandolph commented 3 years ago

Update after a week is that the merged fix has been working fine for me, and a few other people I know that I are using it actively.

bbatsov commented 3 years ago

Good to know! Thanks for following up!