jasongilman / proto-repl

A Clojure Development Environment package for the Atom editor
https://atom.io/packages/proto-repl
MIT License
565 stars 50 forks source link

Only wrap code in a do block when there are multiple statements #250

Closed rgdelato closed 7 years ago

rgdelato commented 7 years ago

It looks like the reason that sending commands such as :cljs/quit to Figwheel through Proto REPL doesn't work is because it's being sent as (do :cljs/quit), which doesn't actually quit the Figwheel REPL.

This PR adds a check to the to-be-sent code to count how many statements it contains and doesn't wrap it in a do block if the code is a single statement.

With this change, I'm able to connect to Figwheel, run Figwheel's control commands, and close it with :cljs/quit!

Related to issue #54

rgdelato commented 7 years ago

Update: After further testing, this change doesn't quite work how I'd hoped. It doesn't account for multiple statements without delimiters, or delimiters inside of strings, such as:

"deck" :of 52 ;;=> doesn't wrap in do block
"a string )}] just a string" ;;=> wraps in do block

...this doesn't seem to break the REPL in any way, but the check definitely isn't perfect.

jasongilman commented 7 years ago

Thanks for working on this. I think you're right that a different solution would be better. At first I was thinking we could just look for specifically for :cljs/quit. I think we could expand that a bit by not wrapping in a do block if the string matches a white listed regular expression

#"^[A-Za-z0-9\-!?.<>:/*=+_]+$"

That would match most Clojure symbols and keywords but exclude things with spaces, braces, and other reader macro type things. There might be an official regular expression for matching Clojure symbols and keywords and that would be ok as well.

What do you think of that solution? That would be a bit simpler.

rgdelato commented 7 years ago

I would definitely prefer a simpler solution and checking for a symbol/keyword would work for at least being able to quit out of Figwheel's REPL at all, which is currently my biggest problem. But unfortunately, it's not just the :cljs/quit command that doesn't work when wrapped in a do, it's pretty much all of Figwheel's control functions that don't work in do blocks (and that work correctly without do blocks):

screen shot 2017-05-23 at 4 42 31 pm

...maybe if we can assume that control functions are all single un-nested calls, it would be possible to use regex to check for either a single symbol/keyword or also a single call?

/^\s*\([^\(\)]+\)\s*$/g

(This should match (foo bar baz), but not (foo) (bar) or (foo (bar)))

rgdelato commented 7 years ago

Okay, I've committed a change that instead only checks the incoming code by running these two regexes.

rgdelato commented 7 years ago

@jasongilman Just wanted to check in since it's been a little bit. Is there anything more that you need from me on this PR?

jasongilman commented 7 years ago

Sorry it took so long for me to get back to this. The updated code looks like it will work. I'll do some testing and make sure it works.