GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — repl: preprocess only for defaultEval by princejwesley · Pull Request #9752 · nodejs/node

Conversation

princejwesley commented Nov 22, 2016

Copy link

Copy Markdown

Contributor

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

repl

Description of change

Code preprocessing is applicable only for default
eval function. Therefore, Moved preprocess function
invocation inside defaultEval function.

Fixes: #9743

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label

Nov 22, 2016

addaleax approved these changes

Nov 22, 2016

View reviewed changes

addaleax left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM pending CI, thank you for doing this :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/parallel/test-repl-eval.js Outdated

addaleax Nov 22, 2016

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can I suggest that you add a comment describing why this particular input was chosen?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

princejwesley Nov 22, 2016

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sure 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

princejwesley commented Nov 22, 2016

Copy link

Copy Markdown

Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/4954/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

YarnSphere reviewed

Nov 23, 2016

View reviewed changes

Comment thread

lib/repl.js Outdated

YarnSphere Nov 23, 2016

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This appending of the \n isn't needed for custom eval functions?
Because it was being appended before.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

princejwesley Nov 23, 2016

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@nunocastromartins Good point. \n was appended for most of the cases and it was done in same preprocess. I'll add \n for all cases as if eval function receives what user entered including the final enter.

CC: @Fishrock123

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Fishrock123 commented Nov 23, 2016 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

princejwesley mentioned this pull request

Nov 30, 2016

Generators in REPL can't be specified using function *gen() {} #9850

Closed

not-an-aardvark mentioned this pull request

Dec 2, 2016

repl: fix generator function preprocessing #9852

Merged

3 tasks done

princejwesley commented Dec 2, 2016

Copy link

Copy Markdown

Contributor Author

@nodejs/collaborators ping

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax approved these changes

Dec 2, 2016

View reviewed changes

addaleax left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Still LGTM

(sorry, I was a bit busy here at NINA)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          repl: preprocess only for defaultEval

b345fab

Code preprocessing is applicable only for default
eval function. Therefore, Moved `preprocess` function
invocation inside `defaultEval` function.

Fixes: nodejs#9743
PR-URL: nodejs#9752
Reviewed-By: Anna Henningsen <[email protected]>

princejwesley force-pushed the repl.9743 branch from ed4179b to b345fab Compare

December 3, 2016 20:42

princejwesley commented Dec 3, 2016

Copy link

Copy Markdown

Contributor Author

Rebased, CI again: https://ci.nodejs.org/job/node-test-pull-request/5157/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

princejwesley added a commit that referenced this pull request

Dec 5, 2016


          repl: preprocess only for defaultEval

9366b8d

Code preprocessing is applicable only for default
eval function. Therefore, Moved `preprocess` function
invocation inside `defaultEval` function.

Fixes: #9743
PR-URL: #9752
Reviewed-By: Anna Henningsen <[email protected]>

princejwesley commented Dec 5, 2016

Copy link

Copy Markdown

Contributor Author

Landed in 9366b8

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

princejwesley closed this

Dec 5, 2016

Fishrock123 pushed a commit that referenced this pull request

Dec 5, 2016


          repl: preprocess only for defaultEval

6087e36

Code preprocessing is applicable only for default
eval function. Therefore, Moved `preprocess` function
invocation inside `defaultEval` function.

Fixes: #9743
PR-URL: #9752
Reviewed-By: Anna Henningsen <[email protected]>

Fishrock123 mentioned this pull request

Dec 5, 2016

v7.2.1 proposal #10127

Merged

2 tasks done

jmdarling pushed a commit to jmdarling/node that referenced this pull request

Dec 8, 2016


          repl: preprocess only for defaultEval

f908538

Code preprocessing is applicable only for default
eval function. Therefore, Moved `preprocess` function
invocation inside `defaultEval` function.

Fixes: nodejs#9743
PR-URL: nodejs#9752
Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins added dont-land-on-v4.x labels

Dec 20, 2016

MylesBorins pushed a commit that referenced this pull request

Dec 20, 2016


          repl: preprocess only for defaultEval

86efc93

Code preprocessing is applicable only for default
eval function. Therefore, Moved `preprocess` function
invocation inside `defaultEval` function.

Fixes: #9743
PR-URL: #9752
Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins pushed a commit that referenced this pull request

Dec 21, 2016


          repl: preprocess only for defaultEval

aed2f35

Code preprocessing is applicable only for default
eval function. Therefore, Moved `preprocess` function
invocation inside `defaultEval` function.

Fixes: #9743
PR-URL: #9752
Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins mentioned this pull request

Dec 21, 2016

V6.9.3 proposal #10394

Merged

Reviewers

addaleax

addaleax approved these changes

+1 more reviewer

YarnSphere

YarnSphere left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

repl

Issues and PRs related to the REPL subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Node REPL preprocessing input string for custom eval functions

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

princejwesley commented Nov 22, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax Nov 22, 2016

Choose a reason for hiding this comment

Uh oh!

princejwesley Nov 22, 2016

Choose a reason for hiding this comment

Uh oh!

princejwesley commented Nov 22, 2016

Uh oh!

YarnSphere Nov 23, 2016

Choose a reason for hiding this comment

Uh oh!

princejwesley Nov 23, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

princejwesley commented Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

princejwesley commented Nov 27, 2016

Uh oh!

princejwesley commented Nov 28, 2016

Uh oh!

princejwesley commented Nov 30, 2016

Uh oh!

princejwesley commented Dec 2, 2016

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

princejwesley commented Dec 3, 2016

Uh oh!

princejwesley commented Dec 5, 2016

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Fishrock123 commented Nov 23, 2016 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

princejwesley commented Nov 23, 2016 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.