Skip to content

[Fix #152] Sanitize should only remove whitespace at the end of a command #165

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

Merged
merged 2 commits into from
Feb 13, 2021

Conversation

arichiardi
Copy link
Contributor

This patch makes sure that we trim at the right of the sanitized command only.

I am open to get rid completely of the sanitation but I do not have the time
now for trying on all the platform.


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][1]
  • The new code is not generating bytecode or M-x checkdoc warnings
  • [] You've updated the changelog (if adding/changing user-visible functionality)
  • [] You've updated the readme (if adding/changing user-visible functionality)

This also gets rid of one step in the CircleCI configuration.
@dotemacs
Copy link
Contributor

dotemacs commented Jun 3, 2019

I do not have the time now for trying on all the platform.

In order to help, I've tested:

(count "1   5")

on

  • ClojureCLR 1.9.0
  • Clojure 1.7.0
  • Clojure 1.8.0
  • Clojure 1.9.0
  • Clojure 1.10.0
  • Lumo 1.7.0
  • Planck 2.10.0

They all return 5 as you'd expect.

@bbatsov
Copy link
Member

bbatsov commented Jun 3, 2019

And multi-line evaluations don't mess up the REPL prompt? I recall this conversation was added to avoid having to deal with subprompts that appeared when entering multi-line expressions.

@arichiardi
Copy link
Contributor Author

Thanks for testing! There is also a test 😀

@dotemacs
Copy link
Contributor

dotemacs commented Jun 3, 2019

And multi-line evaluations don't mess up the REPL prompt? I recall this conversation was added to avoid having to deal with subprompts that appeared when entering multi-line expressions.

This is how it looks for Clojure 1.(7|8|9|10).0 & ClojureCLR 1.9.0:

eg.core=> (println 
"foo")
     #_=> foo
nil
eg.core=> 

Where I enter C-q C-j after println, followed by RET at the end of the s-expression.

Tried it out with Lumo & Planck and they behaved OK, but only once(!) the inf-clojure-repl-type was specified prior to the invocation of their REPLs, via .dir-locals.el:

((nil . ((inf-clojure-lein-cmd . "lumo -r")
         (inf-clojure-minor-mode . t)
         (inf-clojure-repl-type . lumo))))

For me, I only need to use this mode for ClojureCLR. For the Clojure on JVM I use CIDER.
I don't really use Planck & I've got some scripts in Lumo, but don't do it on daily basis.
I'd much rather that somebody who is "knee deep" in them check how their REPLs behave for them.

For Joker, I don't use it at all. Even with the above .dir-locals.el modified for Joker's settings, it was still trying to invoke JVM related documentation and throw errors. But once I disabled eldoc-mode, the REPL behaved just fine. Still, I'd like somebody who is into Joker with this mode to bless this before you merge the change.

@arichiardi
Copy link
Contributor Author

I am trying that (count...) thing working with lumo but this is what I see with this patch:


cljs.user=> ("count" "counted?")
cljs.user=> cljs.user=>        #_=>          #_=>                                        #_=>   ("count" "counted?")
cljs.user=> cljs.user=>          #_=>                                        #_=>   cljs.user=>        #_=>          #_=>                                        #_=>   ("test")
cljs.user=> (count "test  ")
6
cljs.user=>          #_=>                                        #_=>   ("count" "counted?")
cljs.user=> cljs.user=> (count "11111")

It seems like each and every \n is generating a subprompt. Probably it is a side-effect and can be treated in some of the post-filter functions that comint provides. It definitely rings a bell. I saw this problem already.

@bbatsov
Copy link
Member

bbatsov commented Jun 3, 2019

Yeah, that's exactly the problem I tried to solve originally by just converting every expression to a single-line form. Probably some post-filter is going to be a better approach, though. I guess it's a common problem with most REPLs/shells. The visual artefacts weren't the real problem - this actually messes up tooling evals as well.

@arichiardi
Copy link
Contributor Author

It does, and in lumo's case it actually crashes the REPL.

@bbatsov bbatsov merged commit e144b33 into master Feb 13, 2021
@arichiardi arichiardi deleted the fix-152 branch February 13, 2021 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants