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.

Sign up for GitHub

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

Jump to bottom

repl: small refactoring #17919

Closed
wants to merge 3 commits into from
Closed

repl: small refactoring #17919

wants to merge 3 commits into from

Conversation

BridgeAR
Copy link
Member

Just a minor refactoring to make the code more pleasant.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

repl

@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Dec 30, 2017
cjihrig
cjihrig reviewed Dec 30, 2017
View reviewed changes
lib/repl.js Outdated
@@ -1136,15 +1125,14 @@ function complete(line, callback) {
}
}
} catch (e) {
//console.log("completion error walking prototype chain:" + e);
// console.log("completion error walking prototype chain:" + e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

lib/repl.js
}
// We're given a duplex readable/writable Stream, like a `net.Socket`
// or a custom object with 2 streams, or the `process` object
input = stream.stdin || stream;
Copy link
Contributor

Choose a reason for hiding this comment

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

It probably doesn't make a difference, but this has the potential to be a behavior change if only one of stream.stdin or stream.stdout were passed in.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, do you want me to change it to the way it was before? I personally do not feel like this is ever going to matter.

maclover7
maclover7 approved these changes Dec 31, 2017
View reviewed changes
Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

LGTM except for @cjihrig's 2 comments.

vikkyconer
vikkyconer suggested changes Jan 1, 2018
View reviewed changes
lib/repl.js Outdated
return expr + '.' + member;
}));
completionGroups.push(
memberGroups[i].map((member) => expr + '.' + member));
}
if (filter) {
filter = expr + '.' + filter;
Copy link

@vikkyconer vikkyconer Jan 1, 2018 •
edited by tniessen
Loading

Choose a reason for hiding this comment

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

filter = expr + '.' + filter;

This can be changed to

filter = `${ expr }.${ filter }`;

Since it removes access usage of +

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

lib/repl.js Outdated
return expr + '.' + member;
}));
completionGroups.push(
memberGroups[i].map((member) => expr + '.' + member));
Copy link

@vikkyconer vikkyconer Jan 1, 2018 •
edited by tniessen
Loading

Choose a reason for hiding this comment

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

const foo = memberGroups.reduce(( acc, memberGroup ) => {
    acc.push(memberGroup.map((member) => `${ expr }.${ member }`));
    return acc;
}, []);
completionGroups.concat(foo);

Don't mind the name foo as could not think of some descent name but this will make the code immutable. Although we are mutating completionGroups array but in next change we can convert it also to immutability

Copy link
Member Author

Choose a reason for hiding this comment

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

That is not necessary out of my perspective. I personally feel the current code is more expressive than using reduce.

@BridgeAR
Copy link
Member Author

BridgeAR commented Jan 2, 2018 •
edited
Loading

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 2, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Jan 4, 2018

Landed in 11dda69, 80988f9

@BridgeAR BridgeAR closed this Jan 4, 2018
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 4, 2018
@BridgeAR
repl: upper case comments first char
80988f9 
PR-URL: nodejs#17919
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 4, 2018
@BridgeAR
repl: refactor code for readability
11dda69 
PR-URL: nodejs#17919
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
@MylesBorins
Copy link
Contributor

this does not land cleanly on v9.x, could you please backport?

as an aside @BridgeAR it appears you are listing the commits in the reverse order they landed... which can make things confusing when landing on release branches. In future could you post the commits in chronological order, or alternatively using the range syntax [headbefore]...[headafter]

@TimothyGu TimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 13, 2018
@BridgeAR BridgeAR mentioned this pull request Mar 8, 2018
Closed
4 tasks
@BridgeAR
Copy link
Member Author

Backported in #19244 (about the order: I fixed that a while ago in my script. It is always in chronological order again).

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 21, 2018
@BridgeAR
repl: upper case comments first char
976823c 
PR-URL: nodejs#17919
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 21, 2018
@BridgeAR
repl: refactor code for readability
6086e71 
PR-URL: nodejs#17919
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2018
@BridgeAR @MylesBorins
repl: upper case comments first char
6997af7 
Backport-PR-URL: #19244
PR-URL: #17919
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2018
@BridgeAR @MylesBorins
repl: refactor code for readability
55f7bbb 
Backport-PR-URL: #19244
PR-URL: #17919
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
@targos targos mentioned this pull request Mar 21, 2018
Merged
@MylesBorins
Copy link
Contributor

Backport requested for 8.x in #19244

@BridgeAR BridgeAR deleted the refactor-repl branch April 1, 2019 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig left review comments

@starkwang starkwang starkwang approved these changes

@vikkyconer vikkyconer vikkyconer approved these changes

@tniessen tniessen tniessen approved these changes

@maclover7 maclover7 maclover7 approved these changes

@XadillaX XadillaX XadillaX approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@lance lance lance approved these changes

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.