feat : Support for multiline statements #16
Conversation
src/repl.js
Outdated
| const result = isRecoverableError(evaluateResult.exceptionDetails.exception, line); | ||
| if (result) { | ||
| this.io.setPrefix('...'); | ||
| this.io.setMultiline(true); |
There was a problem hiding this comment.
setMultiline should handle the ... to keep the prefix consistent (see screenshot) also we have to discard the return value of onLine because it dumps undefined when a new multiline happens (see screenshot)
otherwise this looks pretty great :D
There was a problem hiding this comment.
something a bit more "flowing" might be to return some symbol (IO.kNeedsAnotherLine or something) from onLine to tell the IO to buffer another line
There was a problem hiding this comment.
@devsnek Sure, will work on these things. I don't get your last statement though:
something a bit more "flowing" might be to return some symbol (IO.kNeedsAnotherLine or something) from onLine to tell the IO to buffer another line
|
@devsnek : Hey.. Have updated the PR with review comments fixed. Let me know if it's all good :) I played around and seems to be working fine:
|
src/repl.js
Outdated
| // lets try for recovering | ||
| const result = isRecoverableError(evaluateResult.exceptionDetails.exception, line); | ||
| if (result) { | ||
| this.io.setPrefix('...'); |
There was a problem hiding this comment.
io should set the prefix when it receives $multiline. also the ... needs a space after.
src/repl.js
Outdated
| } else { | ||
| // we tried our best - throw error | ||
| // this.io.setMultiline(false); | ||
| this.io.setPrefix('>'); |
There was a problem hiding this comment.
arrow should be reset by io when multiline ends. also the arrow needs a space after it.
|
@devsnek Thanks, I have done those changes locally, but somehow, I missed to push it. Now I guess it should be alright. Thanks for your time on this. |
|
ping @TimothyGu |
|
Just now realized my git username was wrong in commits; so just rewritten my commit message with right user name. |
src/util.js
Outdated
| @@ -1,6 +1,7 @@ | |||
| 'use strict'; | |||
|
|
|||
| const { isIdentifierStart, isIdentifierChar } = require('acorn'); | |||
| const $multiLine = Symbol('IO.kNeedsAnotherLine'); | |||
There was a problem hiding this comment.
the symbols description should match where the symbol is mounted and under what variable name it is mounted. you should move it to io.js file.
There was a problem hiding this comment.
Sure, will look into this.
There was a problem hiding this comment.
Shouldn't it be multiline (lower and not camel which would suggest multi line)?
src/io.js
Outdated
| @@ -1,6 +1,7 @@ | |||
| 'use strict'; | |||
|
|
|||
| const { emitKeys, CSI, cursorTo } = require('./tty'); | |||
| const multiline = Symbol('IO.kNeedsAnotherLine'); | |||
There was a problem hiding this comment.
please name the variable kNeedsAnotherLine, and if it is supposed to be IO.kNeedsAnotherLine, put it on the IO object. (IO.kNeedsAnotherLine = Symbol('IO.kNeedsAnotherLine')
|
@devsnek Taken care, thanks. Let me know if anything needed from my side. |
|
@devsnek Any update on this? I'm waiting for your reply :) |
Hello,
Added the supported for multiline statements :#15
I have tested locally, it works.
Thoughts please?
Note: Recoverable.js is something I had taken from node core.