Skip to content

repl: refactor to avoid unsafe array iteration#37188

Merged
aduh95 merged 1 commit intonodejs:masterfrom
aduh95:repl-array-iteration
Feb 4, 2021
Merged

repl: refactor to avoid unsafe array iteration#37188
aduh95 merged 1 commit intonodejs:masterfrom
aduh95:repl-array-iteration

Conversation

@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Feb 2, 2021

No description provided.

@nodejs-github-bot nodejs-github-bot added readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem. labels Feb 2, 2021
Lxxyx
Lxxyx reviewed Feb 3, 2021
View reviewed changes
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 3, 2021

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

@Lxxyx Lxxyx added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 3, 2021
@Trott
Copy link
Member

Trott commented Feb 3, 2021

LGTM, but are the test changes unrelated? If not, does it make sense to add a separate test case in the file where we add allowBlockingCompletions: true rather than changing the settings on an existing test case?

@Trott
Copy link
Member

Trott commented Feb 3, 2021

LGTM, but are the test changes unrelated? If not, does it make sense to add a separate test case in the file where we add allowBlockingCompletions: true rather than changing the settings on an existing test case?

(Is the test case adding coverage to the code modified here because it wasn't previously covered?)

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 3, 2021

LGTM, but are the test changes unrelated?

They are related in the sense I used them to spot the use of unsafe iteration.

If not, does it make sense to add a separate test case in the file where we add allowBlockingCompletions: true rather than changing the settings on an existing test case?

Yeah, or maybe run the same test twice: first with allowBlockingCompletions: false and then with allowBlockingCompletions: true?

@Trott
Copy link
Member

Trott commented Feb 3, 2021

Yeah, or maybe run the same test twice: first with allowBlockingCompletions: false and then with allowBlockingCompletions: true?

Yeah, that works.

Trott
Trott approved these changes Feb 3, 2021
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM, although I'd prefer the existing test case configuration be added to rather than modified. But that's not a blocking comment.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 3, 2021

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

@aduh95
repl: refactor to avoid unsafe array iteration
703e566 
PR-URL: nodejs#37188
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@aduh95 aduh95 force-pushed the repl-array-iteration branch from f23e74b to 703e566 Compare February 4, 2021 23:16
@aduh95
Copy link
Contributor Author

aduh95 commented Feb 4, 2021

Landed in 703e566

@aduh95 aduh95 merged commit 703e566 into nodejs:master Feb 4, 2021
@aduh95 aduh95 deleted the repl-array-iteration branch February 4, 2021 23:17
This was referenced Feb 16, 2021
Closed
Merged
@targos targos mentioned this pull request Sep 4, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott approved these changes

@Lxxyx Lxxyx Lxxyx approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. readline Issues and PRs related to the built-in readline module. 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.

6 participants

@aduh95 @nodejs-github-bot @Trott @Lxxyx @RaisinTen @targos