Skip to content
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

Make repl ignore a blank string rather than evaluating it #2235

Merged
merged 1 commit into from
Mar 12, 2018

Conversation

gonewest818
Copy link
Contributor

@gonewest818 gonewest818 commented Mar 10, 2018

As the subject says. The bug was identified and commented on in #1115 (comment)

I'll push some additional changes to this branch to finish up the checklist below.


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 (make test)
  • All code passes the linter (make lint) which is based on elisp-lint and includes
  • 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.

gonewest818 added a commit to gonewest818/cider that referenced this pull request Mar 10, 2018
cider-repl.el Outdated
(string-match-p "\\`[ \t\r\n]*\\'" input))
(cider--nrepl-pprint-request-plist (cider--pretty-print-width))))))
(let ((input (cider-repl--current-input)))
(unless (string= input "") ; don't evaluate an empty string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string-empty-p

gonewest818 added a commit to gonewest818/cider that referenced this pull request Mar 10, 2018
@bbatsov
Copy link
Member

bbatsov commented Mar 11, 2018

LGTM. This doesn't affect printing the next prompt, right? Just want to make sure we weren't doing the printing in an eval handler or something like this.

cider-repl.el Outdated
(string-match-p "\\`[ \t\r\n]*\\'" input))
(cider--nrepl-pprint-request-plist (cider--pretty-print-width))))))
(let ((input (cider-repl--current-input)))
(unless (string-empty-p input) ; don't evaluate an empty string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, I'm curious if this will work properly if you type a few spaces and then press Return in the REPL. Likely not, so probably we need a bit more robust check for no input.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess string-blank-p should do the trick.

gonewest818 added a commit to gonewest818/cider that referenced this pull request Mar 11, 2018
@gonewest818
Copy link
Contributor Author

gonewest818 commented Mar 11, 2018

Ok, I adopted string-blank-p and added some logic as well. Now the behavior is as follows.

If the input string contains only whitespace (including newlines) then we clear the input without evaluating, print a fresh prompt, and we do NOT evaluate or add the blank input to the history.

Otherwise, if the input is not blank it is evaluated as usual.

@gonewest818 gonewest818 changed the title Make repl ignore the empty string rather than evaluating "" Make repl ignore a blank string rather than evaluating it Mar 11, 2018
@bbatsov bbatsov merged commit d628e2e into clojure-emacs:master Mar 12, 2018
@bbatsov
Copy link
Member

bbatsov commented Mar 12, 2018

Nicely done! 👍

@gonewest818 gonewest818 deleted the repl-empty-string branch March 12, 2018 01:57
@vspinu
Copy link
Contributor

vspinu commented Jun 11, 2018

I think this change is a "usability" regression. The process might be killed or locked for whatever reason, but my returns go through as if nothing happened. I am so used to sending RETs as a control for hang in all emacs REPLs that I find this new behavior rather disturbing.

What bug does this change fix more concretely? The referenced comment doesn't shed light on the nature of the bug.

@bbatsov
Copy link
Member

bbatsov commented Jun 12, 2018

I considered this a bug because when you press Ret on an empty line technically you haven't entered anything, but you ended up evaluating an empty string. For me the only case in which you should eval an empty string should be typing "" in the REPL (and, of course, this should result in "" being echoed as the output). Long story short - I don't want us to have to think about special cases at all - everything should be consistent.

@bbatsov
Copy link
Member

bbatsov commented Jun 12, 2018

I think this change is a "usability" regression. The process might be killed or locked for whatever reason, but my returns go through as if nothing happened. I am so used to sending RETs as a control for hang in all emacs REPLs that I find this new behavior rather disturbing.

That's an interesting argument that I didn't think of, though. I generally just type something to test this, so it never came to my mind that someone might be testing their REPL in this manner.

@vspinu
Copy link
Contributor

vspinu commented Jun 12, 2018

technically you haven't entered anything, but you ended up evaluating an empty string

Shouldn't this be taken care by nREPL automatically? To my mind the eval op should differentiate between :code "" and :code "\"\"".

I generally just type something to test this

Never came to my mind that someone might be testing their REPL in this manner either :)

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.

4 participants